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

add section for extra info in config #18

Merged
merged 14 commits into from
Nov 20, 2024

Conversation

leifdenby
Copy link
Member

Add section called extra in config which is ignored by mllam-data-prep but can be used to add any extra info the user might want to keep track of. The intention is that this will be used in neural-lam to keep of projection information for now at least

@leifdenby
Copy link
Member Author

Just FIY I have decided that this hacky approach (where an extra section is added that is ignore by the application) isn't something I feel comfortable adding to mllam-data-prep. Instead I have asked @SimonKamuk and @observingClouds to look into implementing #33 so adding projection information can be done properly

@leifdenby leifdenby closed this Nov 8, 2024
@leifdenby
Copy link
Member Author

I am reopening this because it is turning out to be currently impossible to add proper WKT-string based projection info to the config and have that parsed and used in cartopy to create a cartopy.crs.Projection object that can be used for plotting.

I do need to fix that the example I am showing with DANRA had the wrong default globe (as in correct default radius for the globe) relative to what is assumed in DANRA.

I.e. the example in the config of this PR results in the projection being

cartopy.crs.LambertConformal(
    central_latitude=56.7, central_longitude=25, standard_parallels=(56.7,56.7), 
)

Which has the incorrect (default) globe radius.

whereas it should be (thank you @SimonKamuk!):

cartopy.crs.LambertConformal(
    central_latitude=56.7, central_longitude=25, standard_parallels=(56.7,56.7), 
    globe=cartopy.crs.Globe(semimajor_axis=6367470, semiminor_axis=6367470)
)

@leifdenby leifdenby reopened this Nov 19, 2024
@leifdenby leifdenby added this to the v0.5.0 milestone Nov 20, 2024
@leifdenby
Copy link
Member Author

Ok, this is ready for review now @observingClouds 🎁

I added some rudimentary functionality to enable support of multiple schema versions and also wrote up my current plans for schema versioning in the bottom of the README. For now I suggest we inline in the code with comments simply keep track of what schema versions the current version of mllam-data-prep supports (in this case it is both v0.2.0 and v0.5.0 since the new extra field is optional). And I also added tests to ensure that support is there (by keeping copy of of old configs with revisions with should still support).

Hope this makes sense!

PS. I noticed when #35 and #36 were merged in that the changelog entry went in the wrong place for the former and there's no changelog entry for the latter. But I will clean that up when I prepare the v0.5.0 release. Just in case you felt like the changelog looks quite empty given the git history since the last tag

Copy link
Contributor

@observingClouds observingClouds left a comment

Choose a reason for hiding this comment

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

Looks good to me, the only issue that I see before this can be merged is the correct projection information....once again...

README.md Show resolved Hide resolved
tests/old_config_schema_examples/v0.2.0/example.danra.yaml Outdated Show resolved Hide resolved
example.danra.yaml Outdated Show resolved Hide resolved
@leifdenby
Copy link
Member Author

Thanks for all the help @observingClouds and @SimonKamuk! I am merging this in 😄

@leifdenby leifdenby merged commit 1a01bc0 into mllam:main Nov 20, 2024
5 checks passed
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.

3 participants