Skip to content

Enable individual LUTs #128

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
Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Enable individual LUTs #128

wants to merge 20 commits into from

Conversation

vbrancat
Copy link
Contributor

@vbrancat vbrancat commented Apr 1, 2023 •
edited
Loading

This PR introduces the capability of computing individual correction LUTs by exposing Boolean flag in the schema for each individual correction.

TO DO:

  1. Modify the stack processor to enable/disable individual corrections
  2. Solve QA computation for weather model troposphere

seongsujeong
seongsujeong reviewed Apr 4, 2023
View reviewed changes
Copy link
Contributor

@seongsujeong seongsujeong left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the PR @vbrancat. Here are my couple of comments below

  • Seongsu

@vbrancat
Copy link
Contributor Author

@seongsujeong and @LiangJYu it would be great if you have time to go through this PR

LiangJYu
LiangJYu reviewed Apr 11, 2023
View reviewed changes
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.

In line with using a different init value for correction dict data, what do you think about:

  1. adding a bool dataset in the HDF5 indicating LUT(s) that were enabled?
  2. not writing disabled LUT(s) to HDF5?

src/compass/s1_geocode_slc.py Outdated
Comment on lines 193 to 198
# if cfg.lut_params.enabled:
# # apply tropo corrections if weather file provided
# apply_tropo_corrections = cfg.weather_model_file is not None
# cslc_qa.compute_correction_stats(
# geo_burst_h5, apply_tropo_corrections,
# cfg.tropo_params.delay_type)
Copy link
Contributor

Choose a reason for hiding this comment

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

Will these commented lines be restored? Or temporary debug?

scottstanie
scottstanie reviewed Apr 18, 2023
View reviewed changes
@scottstanie
Copy link
Contributor

The s1_geocode_stack.py will have to be changed to: https://github.com/vbrancat/COMPASS/blob/single_lut/src/compass/s1_geocode_stack.py#L287 this one currently will make invalid YAMLs by having the single bool... not sure if we really want to expose all 6 on/off options to that script though. That seems excessive, especially if we expect most people will want the corrections (assuming they all improve things)

scottstanie
scottstanie reviewed Apr 18, 2023
View reviewed changes
@LiangJYu
Copy link
Contributor

The paths from my changes are a mess. I'll fix them as soon as #148 gets merged into main as I see no point in fixing them now and after another merge with main.

@LiangJYu LiangJYu changed the title Enable individual LUTs WIP Enable individual LUTs May 22, 2023
@LiangJYu
restore data type assignment
2ac7fed 
path updates
fix dataset access to correction flags
update documentation
remove unused code
@LiangJYu LiangJYu changed the title WIP Enable individual LUTs Enable individual LUTs May 22, 2023
@LiangJYu
Copy link
Contributor

Unable replicate failed unit test locally with a new environment created from docker/specifile.txt

Currently investigating why local docker fails differently than CircleCI.

LiangJYu added 3 commits May 23, 2023 09:34
@LiangJYu
fix paths
679674c 
update/add comments and variable names
@LiangJYu
Revert "Merge branch 'main' into single_lut"
0b12860 
This reverts commit 3c4d8d4, reversing
changes made to 31935c4.
@LiangJYu
Merge branch 'main' into single_lut
21685ef
@LiangJYu LiangJYu mentioned this pull request May 25, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LiangJYu LiangJYu LiangJYu left review comments

@scottstanie scottstanie scottstanie left review comments

@seongsujeong seongsujeong seongsujeong left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vbrancat @scottstanie @LiangJYu @seongsujeong