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

Placeholder for geocoded noise LUT #119

Merged
merged 7 commits into from
Mar 22, 2023

Conversation

seongsujeong
Copy link
Contributor

This PR is to add the geocoded noise LUT into the metadata. Currently the noise information is included in the product as radargrid. It is necessary to provide the geocoded LUT, rather than radargrid LUT, to make this information useful.

This addition is basically the repetition of #114 (geocoded calibration LUT), also modification of the noise information's location.

Copy link
Contributor

@vbrancat vbrancat left a comment

Choose a reason for hiding this comment

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

Left some comments. It would be good if we can avoid code redundancy


# define the geogrid for calbration LUT
noise_radargrid = radar_grid.multilook(dec_factor,
dec_factor)
Copy link
Contributor

Choose a reason for hiding this comment

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

adjust formatting

iters = cfg.geo2rdr_params.numiter
scratch_path = out_paths.scratch_directory

# Designate radiometric calibration parameter to geocode
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this might be a copy & paste error

@@ -258,6 +258,133 @@ def geocode_calibration_luts(geo_burst_h5, burst, cfg,
del geocoded_cal_lut_raster



def geocode_noise_luts(geo_burst_h5, burst, cfg,
Copy link
Contributor

Choose a reason for hiding this comment

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

This function has a lot of code repetition with the calibration LUT function. I am wondering whether is possible to consolidate the two in a single function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with your concern. The code redundancy has reduced thanks to @LiangJYu.

Copy link
Contributor

@LiangJYu LiangJYu left a comment

Choose a reason for hiding this comment

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

LGTM. My comments are for things I left out in my PR to your PR.

src/compass/s1_geocode_metadata.py Outdated Show resolved Hide resolved
src/compass/s1_geocode_metadata.py Outdated Show resolved Hide resolved
Co-authored-by: Liang Yu <liangjyu@gmail.com>
Copy link
Contributor

@vbrancat vbrancat left a comment

Choose a reason for hiding this comment

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

Thanks for having reorganized the code

@seongsujeong seongsujeong merged commit 68df0c1 into opera-adt:main Mar 22, 2023
seongsujeong added a commit to seongsujeong/COMPASS that referenced this pull request Mar 31, 2023
* noise info moved to `metadata/noise_information`

* `geocode_noise_luts` implemented

* removed radargrid-based information in `noise_information`

* codacy issue

* reduce duplicate code

* fix on `lut_path` definition

* Apply suggestions from code review

Co-authored-by: Liang Yu <liangjyu@gmail.com>

---------

Co-authored-by: Seongsu Jeong <seongsu.jeong@jpl.nasa.gov>
Co-authored-by: Liang Yu <liangjyu@gmail.com>
@seongsujeong seongsujeong deleted the geocoded_noise_lut branch May 26, 2023 00:08
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.

3 participants