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.

Sign up for GitHub

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

Jump to bottom

[Bug]: Inconsistent correction grid dimensions #105

Closed
vbrancat opened this issue Mar 4, 2023 · 0 comments · Fixed by #107
Closed

[Bug]: Inconsistent correction grid dimensions #105

vbrancat opened this issue Mar 4, 2023 · 0 comments · Fixed by #107
Assignees
@scottstanie
Labels
bug Something isn't working needs triage Issue requires triage to proceed

Comments

@vbrancat
Copy link
Contributor

vbrancat commented Mar 4, 2023

Checked for duplicates

Yes - I've already checked

Describe the bug

Some correction grids within the s1-reader (e.g. bistatic delay and azimuth FM-rate mismatch) may have inconsistent dimensions.

For example the correction grid of the azimuth FM-rate mismatch may be computed using the decimation function of the radar grid reported here https://github.com/opera-adt/s1-reader/blob/0e0c7816a9b22b057a769519d302cd62e0a4e346/src/s1reader/s1_burst_slc.py#L267.

However, the correction grid for the bistatic delay is computed using the function reported here https://github.com/opera-adt/s1-reader/blob/0e0c7816a9b22b057a769519d302cd62e0a4e346/src/s1reader/s1_burst_slc.py#L531. The difference between the two function is using np.ceil when computing the dimension in both slant range and azimuth directions.

This causes the two grid to be off by one pixel. For example, when the two grids are added together:

Traceback (most recent call last):
  File "/mnt/aurora-r0/vbrancat/codes/COMPASS/src/compass/s1_cslc.py", line 54, in <module>
    main()
  File "/mnt/aurora-r0/vbrancat/codes/COMPASS/src/compass/s1_cslc.py", line 49, in main
    run(parser.run_config_path, parser.args.grid_type)
  File "/mnt/aurora-r0/vbrancat/codes/COMPASS/src/compass/s1_cslc.py", line 43, in run
    s1_geocode_slc.run(cfg)
  File "/mnt/aurora-r0/vbrancat/codes/miniconda3/envs/compass/lib/python3.11/site-packages/compass/s1_geocode_slc.py", line 82, in run
    rg_lut, az_lut = cumulative_correction_luts(burst,
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/mnt/aurora-r0/vbrancat/codes/miniconda3/envs/compass/lib/python3.11/site-packages/compass/utils/lut.py", line 60, in cumulative_correction_luts
    az_lut_data = -(bistatic_delay.data + az_fm_mismatch.data)
                    ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
ValueError: operands could not be broadcast together with shapes (110,408) (109,407)

What did you expect?

I expected the dimensions of all the correction grid contained in the repository to be consistent

Reproducible steps

No response

Environment

No response
@vbrancat vbrancat added bug Something isn't working needs triage Issue requires triage to proceed labels Mar 4, 2023
@vbrancat vbrancat assigned vbrancat and scottstanie and unassigned vbrancat Mar 4, 2023
@vbrancat vbrancat mentioned this issue Mar 6, 2023
Closed
scottstanie added a commit to scottstanie/s1-reader that referenced this issue Mar 6, 2023
@scottstanie
use radargrid step parameters to simplify _steps_to_vecs
d827d3d 
Now that the function `self.as_isce3_radargrid` has parameters for step sizes, we dont need to redo the logic to compute the coordinates.

This should fix isce-framework#105 where the differing logic causes different coordinates.

It also allows us to remove redundant error checking that happens in the `as_isce3_radaragrid` function
@scottstanie scottstanie mentioned this issue Mar 6, 2023
Merged
scottstanie added a commit that referenced this issue Mar 7, 2023
@scottstanie
use radargrid step parameters to simplify _steps_to_vecs (#107)
c2c53d2 
* use radargrid step parameters to simplify `_steps_to_vecs`

Now that the function `self.as_isce3_radargrid` has parameters for step sizes, we dont need to redo the logic to compute the coordinates.

This should fix #105 where the differing logic causes different coordinates.

It also allows us to remove redundant error checking that happens in the `as_isce3_radaragrid` function

* fix param name to be rg_
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@scottstanie scottstanie

Labels
bug Something isn't working needs triage Issue requires triage to proceed
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

use radargrid step parameters to simplify _steps_to_vecs scottstanie/s1-reader
2 participants
@scottstanie @vbrancat