-
Notifications
You must be signed in to change notification settings - Fork 18
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
Correctly populate product fill values in geocoded datasets #167
base: main
Are you sure you want to change the base?
Conversation
updated docstring
rename grid to data in applicable dataset objects
@LiangJYu do you mind update this branch? So we can go ahead and test it. Thanks. |
Unit tests pass with most current code on FN |
@LiangJYu do you mind merging the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LiangJYu, thanks for this PR. I am a bit lost. I see you implemented a functionality to retrieve the fill value for datasets with different data types. However, I don't see anywhere in the code that these fill values are plugged into the product metadata. Can you clarify?
Also, it would be great to come up with an automated way of understanding which fill value has been used for each data type instead of hard-coding them. If this is not possible, can we group all the fill value somewhere more visible?
All the changes have been merged away against |
make fill value logic more robust rename grid_group to data_group
@LiangJYu can we quickly address these comments by EOD? |
@seongsujeong @vbrancat Comments have been addressed. Products look good from my testing with the Peru dataset. Please have a look. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind merging the main
branch? It would be much easier for testing. Merci.
@LiangJYu I have processed a CSLC-S1 product using this PR and inspected it using HDF5. In the HDF5 viewer, I am not able to see the _FillValue as an attribute of 2D datasets. Can we insert as attribute for all the 2D datasets (timing corrections included)? Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me but for consistency, we should add fill values also to the timing corrections since these are 2D datasets. @seongsujeong, please, go ahead and review this PR as well.
Safe to assume NaN as fill value for all correction values? Apologies, I don't know them off the top of my head. |
Don't wanna slow this down if you're about to try and get it merged, but if you wanted to cut down on some of the code, you might just be able to use the numpy dtype 'kind' In [13]: dts = ['complex64', 'float64', 'float32', 'uint8']
In [14]: for dt in dts:
...: print(dt, ':', np.dtype(dt).kind)
...:
complex64 : c
float64 : f
float32 : f
uint8 : u that's how they do this in h5netcdf: https://github.com/h5netcdf/h5netcdf/blob/2951cf8ea3174dde5663451484bde0cebaa535d0/h5netcdf/legacyapi.py#L24-L30 |
@seongsujeong can you go ahead and have another look at this PR? Thanks |
This PR:
init_geocoded_dataset
tocreate_geocoded_dataset
h5py.Dataset
variables withgrid
prefix todata
prefix