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

Use image center when computing flying height #485

Merged
merged 1 commit into from
May 14, 2024

Conversation

oleg-alexandrov
Copy link
Collaborator

@oleg-alexandrov oleg-alexandrov commented May 14, 2024

When a CSM linescan model is loaded, some auxiliary info is computed, including the GSD, reference point, and flying height.

All but the last one use the image center (half image dimensions) for the computations. But the flying height uses time 0. Time 0 is not well-defined for a linescan sensor. One should use either the starting time as set in the metadata, or the time at starting line, or some time that is relevant to the given sensor.

I ran into this when creating custom linescan models. The resulting flying height then ends up being quite off, depending on what the starting time is.

The proposed fix uses the image center time to find the flying height, for consistency with GSD and reference point calculations.

The effect of this fix in practice is very small, unless one does unusual things.

Looks like all tests still pass after the fix.

Licensing

This project is mostly composed of free and unencumbered software released into the public domain, and we are unlikely to accept contributions that are not also released into the public domain. Somewhere near the top of each file should have these words:

This work is free and unencumbered software released into the public domain. In jurisdictions that recognize copyright laws, the author or authors of this software dedicate any and all copyright interest in the software to the public domain.

  • I dedicate any and all copyright interest in this software to the public domain. I make this dedication for the benefit of the public at large and to the detriment of my heirs and successors. I intend this dedication to be an overt act of relinquishment in perpetuity of all present and future rights to this software under copyright law.

@acpaquette
Copy link
Collaborator

@oleg-alexandrov This looks fine, my understanding with USGSCSM is that time "0" is the middle of the line scan exposure since the times get offset by the center time. So your beginning times should be negative approaching 0 at the middle, then increasing and remaining positive till the end of the image. It also seems weird that this change didn't effect the tests at all. Is there some dataset or situation where this change improves/accurately calculates the flying height?

@oleg-alexandrov
Copy link
Collaborator Author

oleg-alexandrov commented May 14, 2024

After looking through the code, I can confirm that the starting time that gets saved to the model state file, which is m_t0Ephem, is indeed such that time 0 is the middle of the line. So, that is not the actual clock time, but got shifted internally. So, the code is consistent.

The problem shows up if one starts with a model state file, which is perfectly self-consistent, and modifies m_t0Ephem, m_t0Quat, and m_intTimeStartTimes to a new value, and forces computation of GSD, reference point, and flying height.

Then, the first two work fine, but the latter does not, because time 0 is now out of sync with starting time.

This also explains why no tests are failing, as the problem shows up if one creates a model state file directly, rather than starting with provided raw clock, etc.

I need to be able to create such model state files because I use CSM with synthetic data, when it would be too much work to imitate all the process starting with querying spice, J2000, etc., and model state files are supposed anyway to be able to hold a valid model.

Here's how to reproduce that the current logic has this issue:

  • Go to usgscsm/build
  • Run ./usgscsm_cam_test --model ../tests/data/ctxLineScan.json --output-model-state model1.json. This will create model1.json.
  • Consistently replace all starting times, which are -10.57126396894455, with -1000.57126396894455 in model1.json. There are 3 occurences (intTimeLines, m_t0_ephem, m_t0_quat)
  • Force recomputation of GSD and flying height. For that, edit model1.json and set "m_flyingHeight": 1000.0, "m_gsd": 1.0.
  • Run: ./usgscsm_cam_test --model model1.json --output-model-state model2.json

This will now result in a bad flying height. Before, it was 315 km, now it will be 3415 km.

@acpaquette
Copy link
Collaborator

acpaquette commented May 14, 2024

Sorry, just trying to understand what we are doing here.

It worries me that we are manually editing times that apply to ephemeris data. By adjusting the start time are we assuming that the interpolation will correctly apply the motion of the spacecraft to whatever time we set?

I am trying to get a scope for the above justification. It seems like we would need to recompute ephemeris times anyway if we are going to go back in time 990 seconds.

If you avoiding changing the starting times, then the flying height and GSD are recomputed correctly by USGSCSM

@oleg-alexandrov
Copy link
Collaborator Author

What you say is correct.

The question is if there exists a precise standard for model state files that spells out all the conventions. Is time 0 required to be at the center of the line? Or can one take a model state file, and shift all times forward by the same amount, and still be guaranteed to get the same result?

For now I think there is no standard either way, and what exists is for convenience of implementation. Then, we could as well apply the change I am suggesting, which explicitly sets the center of the image as the locations relative to which various calculations take place.

@acpaquette
Copy link
Collaborator

Cool, just wanted to make sure that this is a bit of an abuse of the model state but glad it exposed a potential bug!

@oleg-alexandrov oleg-alexandrov merged commit 805cea4 into DOI-USGS:main May 14, 2024
13 checks passed
@oleg-alexandrov
Copy link
Collaborator Author

Thanks, Adam.

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.

2 participants