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

Solid Earth Tides #91

Merged
merged 16 commits into from
Feb 7, 2023
Merged

Solid Earth Tides #91

merged 16 commits into from
Feb 7, 2023

Conversation

cmarshak
Copy link
Collaborator

Computes solid earth tides via pysolid and this example by @sssangha.

Basic notebook to demonstrate how to use and plots.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@cmarshak cmarshak requested a review from dbekaert February 4, 2023 00:53
@cmarshak
Copy link
Collaborator Author

cmarshak commented Feb 4, 2023

Here is a sample product with the Solid Earth Tide included: link.

It is correctly georeferenced as noted in this screenshot:

image

This is what the solid earth tide looks like in QGIS with an ESRI tile background

image

@cmarshak
Copy link
Collaborator Author

cmarshak commented Feb 4, 2023

Putting this here for reference: #84.

Also this notebook, which serves as an integration test/documentation, adapted from Sim's repository.

@dbekaert
Copy link
Collaborator

dbekaert commented Feb 4, 2023

@sssangha @cmarshak

  1. Please make proper use of longitude and latitude instead of x and y. We want the product to have a consistent feel to users across different groups. All for the GUNW is in WGS84 so lon lat should be fine to use.
  2. Update the fill value not to be a nan; consistent feel with our other variable groups.
  3. I note the use of double. Please justify the impact on accuracy versus float.
  4. In a separate issue tickets or PR to make it documented:
  • confirm correct use of sign when propagated up to mintpy
  • Confirm stich-ability
  • Confirm for a ts stack it gives same results as if you use mintpy internal .

@cmarshak
Copy link
Collaborator Author

cmarshak commented Feb 6, 2023

Please make proper use of longitude and latitude instead of x and y. We want the product to have a consistent feel to users across different groups. All for the GUNW is in WGS84 so lon lat should be fine to use.

Done - see the sample below or these tests

Update the fill value not to be a nan; consistent feel with our other variable groups.

Done; same as above.

I note the use of double. Please justify the impact on accuracy versus float.

Sim uses the imaging geometry dataset (coordinates and SAR look angles) to generate the SET layer. It is cleanest to literally copy the format of this dataset to this layer (not to mention ensuring consistency with this similar layer). We would have to redo the code if we wanted float within the metadata (see this). Honestly, I do not think there is any issue with accuracy as when xarray reads it is seen as the array is seen as float64 type. This aligns with the fact that (I believe) the GUNW is an hdf5 dataset (requires h5py to read/write) and the double and float types correspond to 32 and 64 bit precision, respectively (see here).

Here is an example of the updates you requested on a 2.0.4 dataset: link

@sssangha
Copy link
Collaborator

sssangha commented Feb 6, 2023

@cmarshak following our convo last week, I have pushed some updates to the dependencies in the env file. @mgovorcin and myself still need to work on the interfacing of ARIA-tools and mintpy to run formal end-to-end tests, but I'm hoping in the meantime to get an independent hack validation to verify it's consistency with the internal mintpy workflow.

Also the SET hyp3 integration PR is ready, which follows same template as the PR which introduced the ionosphere estimate option, where by default the new option is set to default until we do more testing.

@dbekaert
Copy link
Collaborator

dbekaert commented Feb 6, 2023

@cmarshak Please inspect the layer, you will see that the geometry meta-date are all float. The only double we have is for lon lat and heights.

  • For the example provided, confirm lon lat changes.
  • Fill value is still Nan

@cmarshak
Copy link
Collaborator Author

cmarshak commented Feb 6, 2023

For the example provided, confirm lon lat changes.

Screenshot:
image

Code: https://github.com/ACCESS-Cloud-Based-InSAR/DockerizedTopsApp/blob/set/isce2_topsapp/solid_earth_tides.py#L140-L142

Fill value is still Nan

Sorry - misread - thought you wanted it to be nan. Looks like several rasters have nodata as 0 (except connectedComponents which is -1). I don't know what this rasters range is for the solidEarthTide - if this raster's pixels can potentially includes 0, then we should use something else. Just say what you want and we can get the correct nodata.

@dbekaert
Copy link
Collaborator

dbekaert commented Feb 7, 2023

It very unlikely that it will be exactly 0. @sssangha thoughts?

@cmarshak
Copy link
Collaborator Author

cmarshak commented Feb 7, 2023

  • I made the nodata 0 as requested and updated the test to reflect that. We can change it back to np.nan if Sim feels otherwise.
  • Also, I was able to use astype to update the type of the group to float32 in xarray, which now shows as float in panoply. Hope this keeps the product more consistent as desired.

@cmarshak
Copy link
Collaborator Author

cmarshak commented Feb 7, 2023

Here is the example link

@sssangha
Copy link
Collaborator

sssangha commented Feb 7, 2023

@dbekaert I agree unlikely to matter, but fyi looks like mintpy does actually set the no_data_value to nan in their SET stacks:

NO_DATA_VALUE none

Here is confirmation in the Mintpy source code that None is assigned as nan.

So honestly I think to maintain internal consistency with Mintpy would be best to revert to np.nan. Sorry @cmarshak for the confusion. Thoughts though @dbekaert ?

@cmarshak
Copy link
Collaborator Author

cmarshak commented Feb 7, 2023

I reverted nodata to np.nan and re-ran the notebook. Hope this clears the way @sssangha.

Thanks - hope this can be integrated soon.

@dbekaert
Copy link
Collaborator

dbekaert commented Feb 7, 2023 via email

@dbekaert
Copy link
Collaborator

dbekaert commented Feb 7, 2023 via email

@cmarshak cmarshak enabled auto-merge February 7, 2023 02:30
@cmarshak
Copy link
Collaborator Author

cmarshak commented Feb 7, 2023

@dbekaert - once you approve - it will automatically be merged (or anyone else outside of Sim and me).

@sssangha
Copy link
Collaborator

sssangha commented Feb 7, 2023

Apologies again for the confusion @cmarshak , after discussion with @dbekaert we agreed it is best to be internally consistent with other variable groups. We could handle nan assignment with mintpy -- if necessary -- through the prep_aria pipeline.

I've re-reverted your changes to reflect a nodata assignment of 0.

Copy link
Collaborator

@dbekaert dbekaert left a comment

Choose a reason for hiding this comment

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

Lgtm

@cmarshak cmarshak merged commit c8aafbb into dev Feb 7, 2023
@cmarshak cmarshak deleted the set branch February 7, 2023 03:59
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.

4 participants