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

Naming of Area definitions in yaml file #2310

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

BENR0
Copy link
Collaborator

@BENR0 BENR0 commented Dec 5, 2022

While working on #2167 I noticed that there were two areas defined in the yaml file with the same name, at least when ignoring capitalization. While this is suboptimal with respect to aforementioned PR (sphinx complaining about duplicate labels), I think this also should be avoided in general.

This PR should be base for a discussion about naming conventions for area definitions. Furthermore, at least suggested from the comments in the area yaml file, it seems there are some "deprecated" area definitions present.

To get the discussion started I think it would be nice to have something like the following for area names:
<region><projection?><resolution?>

@codecov
Copy link

codecov bot commented Dec 5, 2022

Codecov Report

Merging #2310 (3a703f1) into main (a040a25) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2310      +/-   ##
==========================================
- Coverage   94.35%   94.33%   -0.02%     
==========================================
  Files         310      310              
  Lines       46575    46575              
==========================================
- Hits        43947    43938       -9     
- Misses       2628     2637       +9     
Flag Coverage Δ
behaviourtests 4.59% <ø> (-0.01%) ⬇️
unittests 94.98% <ø> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
satpy/tests/reader_tests/test_vii_l2_nc.py 93.93% <0.00%> (-6.07%) ⬇️
satpy/tests/reader_tests/test_vii_l1b_nc.py 97.18% <0.00%> (-2.82%) ⬇️
satpy/tests/reader_tests/test_nwcsaf_msg.py 98.19% <0.00%> (-1.81%) ⬇️
satpy/tests/reader_tests/test_vii_base_nc.py 98.75% <0.00%> (-1.25%) ⬇️
satpy/tests/test_config.py 96.78% <0.00%> (-0.41%) ⬇️

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

@djhoese
Copy link
Member

djhoese commented Dec 5, 2022

I'm surprised you even noticed this. Doesn't YAML just overwrite the previously loaded key as it loads the dictionary/mapping?

I agree the names should not match or be confuse-able in general. There is another issue regarding builtin satellite-based area definition naming that has a specific "scheme" being worked on. The other problem is that a lot of these are based on legacy areas from mpop I think. We've also talked about in some of the monthly meetings I think about have builtin areas for specific regions (I don't think there are any US ones currently).

@BENR0
Copy link
Collaborator Author

BENR0 commented Dec 6, 2022

Yes I think duplicate keys get overwritten but in this case the due to different capitalization and keys being case sensitive they both are included. I think regions in general are a good idea at least in the description somewhere. To sort the yaml file by but also from a user perspective if they are included in the area definition table one could sort through those by region.

Is #2238 the issue you @djhoese were thinking about?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Ready for review
Development

Successfully merging this pull request may close these issues.

2 participants