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

WIP ability to load orbit from annotation XML #41

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

LiangJYu
Copy link
Contributor

This PR address #40 and add:

  • ability to load orbit from annotation XML
  • moved orbit file reader to s1_orbit
  • default to annotation orbit

To do:

  • more testing
  • update documentation

moved orbit file reader to s1_orbit
default to annotation orbit
@LiangJYu LiangJYu added the enhancement New feature or request label May 18, 2022
@LiangJYu LiangJYu requested review from hfattahi, yunjunz and vbrancat May 18, 2022 04:06
@hfattahi
Copy link
Contributor

hfattahi commented May 18, 2022

Thank you @LiangJYu , It looks good. I gave it a try and for one case I got the following error message from our isce3 orbit constructor:

ValueError: non-uniform spacing between state vectors encountered - interval between state vector at position 1 and state vector at position 2 is 9.999999 s, expected 10.000001 s

complaining that the orbit is not uniform. We know that we have a very strict threshold in isce3 about checking the orbit uniform sampling. Let's discuss if we should allow users to relax that threshold to be able to load some not very precise orbits at their own risk.

@LiangJYu LiangJYu linked an issue May 18, 2022 that may be closed by this pull request
@LiangJYu
Copy link
Contributor Author

Thank you @LiangJYu , It looks good. I gave it a try and for one case I got the following error message from our isce3 orbit constructor:

ValueError: non-uniform spacing between state vectors encountered - interval between state vector at position 1 and state vector at position 2 is 9.999999 s, expected 10.000001 s

complaining that the orbit is not uniform. We know that we have a very strict threshold in isce3 about checking the orbit uniform sampling. Let's discuss if we should allow users to relax that threshold to be able to load some not very precise orbits at their own risk.

@hfattahi what do you think about modifying the ISCE3 orbit constructors to take a relaxed threshold? It doesn't appear too complicated to do in ISCE3. Here's what I think needs to be done:

  • Utilize the iscClose() that is overload to accepts an user specified threshold.
  • Modify the Orbit constructor and getOrbitTime to accept a user threshold while defaulting to current threshold

@hfattahi
Copy link
Contributor

Thank you @LiangJYu , It looks good. I gave it a try and for one case I got the following error message from our isce3 orbit constructor:

ValueError: non-uniform spacing between state vectors encountered - interval between state vector at position 1 and state vector at position 2 is 9.999999 s, expected 10.000001 s

complaining that the orbit is not uniform. We know that we have a very strict threshold in isce3 about checking the orbit uniform sampling. Let's discuss if we should allow users to relax that threshold to be able to load some not very precise orbits at their own risk.

@hfattahi what do you think about modifying the ISCE3 orbit constructors to take a relaxed threshold? It doesn't appear too complicated to do in ISCE3. Here's what I think needs to be done:

  • Utilize the iscClose() that is overload to accepts an user specified threshold.
  • Modify the Orbit constructor and getOrbitTime to accept a user threshold while defaulting to current threshold

There can be two approaches to solve this issue I guess. One would be to allow irregular state vector sampling and let the orbit interpolator handle things. I believe we already have an open issue in isce3 repo and it will eventually come in.

The other solution is what you suggested. It is simpler but a bit risky. However, the users of these APIs are really advanced developers and they should know what they are doing if they decide to relax the threshold. I'm ok if we allow for a threshold to be passed to the orbit constructor. By default it won't be needed and would use the current strict threshold. Let's discuss on isce3 repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Orbit from header
2 participants