Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Batched mode propagation for satellites with _isimp flag activated #9

Merged
merged 6 commits into from
May 20, 2024

Conversation

Rex2000viola
Copy link
Contributor

I modified the pipeline in order to propagate both satellites with _isimp flag activated and not.

@Sceki
Copy link
Member

Sceki commented May 2, 2024

Hi @Rex2000viola , thanks for the PR :)

Can you please add tests to make sure that the behavior is as intended. You can add them here: https://github.com/esa/dSGP4/blob/master/tests/test_batched_sgp4.py, by adding for instance: def test_isimp_batched(self): .... where you make sure that the behavior of isimp in batch mode is the same as the one without batch mode --> e.g. propagating the TLEs at a few random future times and making sure the outputs correspond.

For that you might need a few TLEs that have the isimp flag active (you can add them to the end of file) similarly to this

@Rex2000viola
Copy link
Contributor Author

Hello @Sceki , as requested, I have inserted the test file using 4 TLEs, two with the flag active and two without. Additionally, I have updated the corrected sgp4_batched file. I apologize for the delay.

Copy link
Member

@Sceki Sceki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Rex2000viola .. thanks! I have added a couple of minor comments, could you please address them?

In the meanwhile, I will let the workflow run as well and there might be fixes to add if those do not pass

dsgp4/sgp4_batched.py Outdated Show resolved Hide resolved
tests/test_batched_sgp4.py Show resolved Hide resolved
@Sceki
Copy link
Member

Sceki commented May 20, 2024

@Rex2000viola I think it's almost ready to merge :) We just miss:

  • removal of the extra return line
  • make sure there is at least one case of isimp==1 in the test

Once these two are pushed I can go ahead and merge

@Rex2000viola
Copy link
Contributor Author

@Sceki I apologize for the wait. I hope it is okay now.

@Sceki
Copy link
Member

Sceki commented May 20, 2024

@Rex2000viola Looks like it's good to go!

Thanks a lot for the contribution, I will merge it!
And good luck with your thesis :)

@Sceki Sceki merged commit e0edc43 into esa:master May 20, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants