pytroll / pyresample

Geospatial image resampling in Python
http://pyresample.readthedocs.org
GNU Lesser General Public License v3.0
346 stars 94 forks source link

Add optional metadata to Pyresample 2.0 AreaDefinition #464

Closed djhoese closed 1 year ago

djhoese commented 1 year ago

As part of one of my projects it has been requested that AreaDefinition's officially support a container of metadata. This is inline with what we had planned for Pyresample 2.0. This PR is the initial support of this functionality.

Note: At the time of writing this is mostly just a refactor of pyresample's future geometry objects to prepare for this functionality.

codecov[bot] commented 1 year ago

Codecov Report

Merging #464 (be2bf83) into main (b8ecc71) will decrease coverage by 0.07%. The diff coverage is 99.15%.

@@            Coverage Diff             @@
##             main     #464      +/-   ##
==========================================
- Coverage   94.33%   94.27%   -0.07%     
==========================================
  Files          74       78       +4     
  Lines       12947    12910      -37     
==========================================
- Hits        12214    12171      -43     
- Misses        733      739       +6     
Flag Coverage Δ
unittests 94.27% <99.15%> (-0.07%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pyresample/area_config.py 90.65% <ø> (ø)
pyresample/ewa/ewa.py 75.86% <ø> (ø)
pyresample/test/test_resamplers/test_nearest.py 100.00% <ø> (ø)
pyresample/test/test_geometry_legacy.py 97.80% <97.80%> (ø)
pyresample/test/test_geometry/test_area.py 99.18% <99.18%> (ø)
pyresample/future/geometry/__init__.py 100.00% <100.00%> (ø)
pyresample/future/geometry/area.py 100.00% <100.00%> (ø)
pyresample/future/geometry/base.py 100.00% <100.00%> (ø)
pyresample/test/conftest.py 98.40% <100.00%> (+0.38%) :arrow_up:
pyresample/test/test_geometry/__init__.py 100.00% <100.00%> (ø)
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

coveralls commented 1 year ago

Coverage Status

Coverage: 93.799% (-0.06%) from 93.862% when pulling be2bf8350e7cdefe6b8925d95f039ad57ee471e3 on djhoese:feature-area-metadata into b8ecc7104cf132f10dee5438eb3049f526199013 on pytroll:main.

djhoese commented 1 year ago

I think my biggest TODO on this is to move the area ID to an optional "name" metadata key in .attrs.

djhoese commented 1 year ago

@mraspaud So my last commits add a new pytest fixture which returns a function that automatically handles keyword argument rearranging and defaulting so the tests in pyresample/test/test_geometry/test_area.py now test both the legacy AreaDefinition class and the new/future one. In these last commits I also removed area_id as a required parameter and made attrs={"name": "some name"} an optional special metadata value along with description.

Some things I thought of or noticed while doing this and/or things that I see as needing to be done. @pnuu @gerritholl @ghiggi I'm curious on your opinions about this as well.

  1. I'd like to convert width and height into a single shape argument.
  2. I'd like to add a "feature flag" to pyresample, probably with a donfig config object, that will allow users to say "all utility methods, use the new class". I need this to complete the test_area.py code testing both classes as it often uses the utility create_area_def which always makes the old area definition classes.
  3. After the new tests are done, I'd like to remove the old area tests module and only use the new one. The new tests aren't named in a "future" way at all (test/test_geometry/test_area.py) so it should be clear to contributors that the AreaDefinition tests are in the new tests module. To really do this I may need to also copy over a lot of SwathDefinition tests which may not be fun, but is going to be needed eventually so :man_shrugging:.

All of these things deserve their own PR in my opinion. It looks like I still need to fix some tests, but otherwise I think I'd like to merge this. Since it only affects the future interfaces merging this shouldn't break anything.

djhoese commented 1 year ago

Unstable CI is failing because matplotlib needs importlib-resources but we explicitly don't install dependencies for unstable packages.

Edit: If we bumped to Python 3.10 it would work fine.

djhoese commented 1 year ago

Thanks for all the feedback @ghiggi, but I think there's been a misunderstanding. The tests added here (with a few exception for the new metadata handling) are all copies from the original AreaDefinition tests. None of these coordinate features/names are new/different. I think we should maybe move the discussion of changes to the AreaDefinition interface somewhere else...I thought I had an issue for that :thinking:

I have my own ideas for changing interfaces and some of us have discussed them in the past. For example, put all coordinate operations (ex. get_lonlats) in a separate coordinate handling class.

Looks like there are no "here's all the area stuff we want to change" issues. We have these for v2.0: https://github.com/pytroll/pyresample/issues?q=is%3Aopen+is%3Aissue+milestone%3Av2.0

djhoese commented 1 year ago

@ghiggi do you have any additional comments about the stuff I did in this PR? Mainly my comment just before your last comment, the idea of metadata in the AreaDefinition (in pyresample 2.0), or the pytest fixture I'm using to test both legacy areas and new areas?

ghiggi commented 1 year ago

Sorry for the very late reply (and the misunderstanding) @djhoese ... I am having super busy days. I have no additional comments ;)

djhoese commented 1 year ago

Strange. Github complained about merge conflicts but when I merged it locally it did it perfectly fine. :man_shrugging:

Now to start on converting all the old tests...

djhoese commented 1 year ago

@mraspaud Ok I think this is ready from the "no duplicate tests" point of view. The tests are much more organized and are all pytest-based now. The remaining tests in the legacy module are for classes that don't currently have a future-pyresample version.