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

Merge gridded data #1674

Merged
merged 31 commits into from
Feb 27, 2024
Merged

Conversation

pat-schmitt
Copy link
Member

@pat-schmitt pat-schmitt commented Jan 15, 2024

This PR is a combined effort of @afisc and myself. The main new functionality is a function for merging gridded_data from several gdirs onto a new 'combined'-grid and optionally adding topography information from a DEM to the new grid.

  • Changes in oggm.core.gis:

    • We had to touch some sensible lines of code here (need a thorough review). Even though it looks like we changed a lot, we mainly outsourced some functionalities from define_glacier_region to reduce code duplication (I think the names are self-explaining): check_dem_source, reproject_dem and get_dem_for_grid. We tried to add a few tests for the new functionalities (in test_prepro), but it is a good sign that all existing tests which use define_glacier_region are not failing (including the graphical tests).

    • Further, we adopted process_dem, read_geotiff_dem and GriddedNcdFile to work with gdir (old behaviour) or you can provide a grid with a filepath.

    • We added a new entity task reproject_gridded_data_variable_to_grid, which is called by workflow.merge_gridded_data (see below) for potential multiprocessing

  • Changes in utils:

    • moved the function combine_grids from graphics to utils
    • in entity_task we added an .unwrapped method, which can be used to execute an entity_task without the overhead. Because this overhead needs a valid gdir and to reduce code duplication we want to use some code with a grid instead of a gdir (e.g. process_dem)
  • Changes in workflow:

    • The main new function of this PR merge_gridded_data: can take gridded data of multiple gdirs, define a new 'combined'-grid and reprojects the data onto this new grid. The function can preserves totals (e.g. volume). Optionally you also could add topography from DEM to that new grid. Finally, the function could take data from different gridded_data-files and merge it (useful for 'distributed'-simulations, see below).
    • added test_merge_gridded_data to test_workflow
  • Changes in sandbox.distribute_2d:

    • New function merge_simulated_thickness: this function is more or less a wrapper for merge_gridded_data and combines all data which could be useful for visualisation from gridded_data and gridded_simulation (this file is created during the redistribution of flowline-data back to the grid). It adds a topography from a DEM and recalculates the bed-topography, using the reprojected thickness after inversion.
    • We renamed distributed_thickness in gridded_simulation to simulated_thickness to avoid confusion with distribued_thickness in gridded_data (which is the thickness after inversion). Alternatively, we can use distributed_inversion_thickness. Any preferences? This needs to be adapted in the tutorials.
    • added test_merge_simulated_thickness to test_models
  • I will work on a tutorial of this new functionality in the next weeks!

I hope I have not forgotten something :). And thanks again @afisc for your help!

  • Tests added/passed
  • Fully documented
  • Create a tutorial or add to an existing tutorial, especially merge_gridded_data
  • Rename distributed_thickness to simulated_thickness in tutorials (if we decide to stick with this name convention)
  • (future addition: add kwarg for adding topography if extend_plot_limit=True in plotting functions from graphics)
  • Entry in whats-new.rst

afisc and others added 22 commits November 27, 2023 15:06
… of it for the new function 'dem_for_combined_grid'
… merge_gridded_data, added test_merge_simulated_thickness
@pep8speaks
Copy link

pep8speaks commented Jan 15, 2024

Hello @pat-schmitt! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-02-27 13:42:41 UTC

@afisc
Copy link
Contributor

afisc commented Jan 15, 2024

@pat-schmitt also thanks from my side for the constant feedback!
Great to see the final PR.

Copy link
Member

@fmaussion fmaussion left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this! Interesting use of unwrapped - I'm pretty sure this is not how it should be done but that's more my problem, not yours. This is looking good I think. To be tested fully requires a global prepro run. We might do one soon enough, but not right now 😅

Feel free to merge @pat-schmitt

@pat-schmitt
Copy link
Member Author

It looks like google updated their images.

Old:
baseline

New:
result

Difference:
result-failed-diff

@fmaussion should I update the image inside of this PR or in a new one? (We also could increase the compare tolerance from 25 to 26 to pass again, I do not know how frequent this updates are...)

@fmaussion
Copy link
Member

The new image is much worse 🙄 - please increase tolerance for now, thanks!

@pat-schmitt pat-schmitt merged commit f7ed63a into OGGM:master Feb 27, 2024
27 checks passed
@pat-schmitt pat-schmitt deleted the visualize_multiple_glaciers branch February 27, 2024 14:15
@bearecinos
Copy link
Member

@patrick I have a very specific questions about the functionality of merge_simulated_thickness() is it possible to obtain the results with lat and lon coordinates instead of (x, y, projection)... ? is there a easy way to transform those coordinates to lat and lon?

@pat-schmitt
Copy link
Member Author

No there is currently no function available for doing this out of the box. However, this could be implemented more or less straightforward by adapting reproject_gridded_data_variable_to_grid https://github.com/OGGM/oggm/blob/master/oggm/core/gis.py#L1891. One could outsource the part of the function which takes one variable as a DataArray and the target_grid (is a salem.grid). Whith this we can define a new function which takes a DataSet and reprojects everything to the target_grid (wich could be in lat, lon coordinates). Maybe something to discuss in the next meeting...

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.

5 participants