openradar / xradar

A tool to work in weather radar data in xarray
https://docs.openradarscience.org/projects/xradar
MIT License
85 stars 17 forks source link

ENH: switch to add recommended metadata #97

Closed egouden closed 1 year ago

egouden commented 1 year ago

Now by default the function to_odim writes only mandatory metadata. There is a new option to add recommended metadata. See ODIM documentation for more information.

Note that only a few recommended metadata can be written at the moment.

codecov[bot] commented 1 year ago

Codecov Report

Merging #97 (0c8ab20) into main (7bd8328) will decrease coverage by 0.36%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #97      +/-   ##
==========================================
- Coverage   88.43%   88.08%   -0.36%     
==========================================
  Files          19       19              
  Lines        3294     3297       +3     
==========================================
- Hits         2913     2904       -9     
- Misses        381      393      +12     
Flag Coverage Δ
unittests 88.08% <100.00%> (-0.36%) :arrow_down:

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

Impacted Files Coverage Δ
xradar/io/export/odim.py 96.77% <100.00%> (+0.07%) :arrow_up:
xradar/io/backends/odim.py 84.98% <0.00%> (-3.15%) :arrow_down:
xradar/io/backends/iris.py 84.85% <0.00%> (+0.09%) :arrow_up:

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

kmuehlbauer commented 1 year ago

Thanks @egouden for pushing this forward.

Would you think it useful to have a kwarg metadata instead, which can have:

egouden commented 1 year ago

Thanks @kmuehlbauer for the quick reply

The problem is a bit more complex. In ODIM 2.2 you have the concept of "recommended" and "prioritized" how attributes. In ODIM 2.3, you have the corresponding "recommended" and "highly desirable". In ODIM 2.4, it becomes "optional" and "mandatory".

I was considering the option of a keyword argument too. However, I see only two options: "mandatory" and "all". Therefore I think that having a switch is more clear for both users and developers.

I just changed the name of the switch to a more explicit "optional_how". It is also better for forward compatibility.

kmuehlbauer commented 1 year ago

Thanks @egouden for the details.

We should elaborate on better version handling in to_odim later. I'll open an issue for that.

egouden commented 1 year ago

The requested changes have been implemented.

kmuehlbauer commented 1 year ago

@egouden Would you mind adding to history.md? We can merge right after that. Thanks!

egouden commented 1 year ago

The history has been updated.

kmuehlbauer commented 1 year ago

Thanks @egouden!

I'd consider those enhancement/fixes of yours worth a new xradar release. WDYT?