Added icon to nwp providers

[gabrielelibardi]

Pull Request

Description

I added icon to the nwp providers. Specifically the changes should allow to an xarray lazily from a list of .zarr or .zarr.zip paths downloaded from here https://huggingface.co/datasets/openclimatefix/dwd-icon-eu.
This pull request is created to address this issue #66 (comment).
In principle this should work even if ones uses remote paths directly to the the .zarr.zip files however because of the many request made to the hugging face server in a short time this may result in a 429 Error. There are ways around this as mentioned in the issue, that have not yet been implemented.

Fixes #

How Has This Been Tested?

see the test_load_icon_eu added to ocf-data-sampler/tests/load/test_load_nwp.py

• [X ] Yes

Checklist:

• My code follows OCF's coding style guidelines
• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• I have checked my code and corrected any misspellings

[Sukh-P]

Thanks for creating this PR and the great work already done on trying to support ICON data in this library!

Something to note is that if this is added in as is that people may assume this library already supports ICON data but without some normalisation constants added and ICON listed as an NWP provider here creating samples from it won't work, so my suggestion is that either this is added in this PR, or in a subsequent PR or this outstanding work is clearly documented in a Github issue or README, thanks!

[gabrielelibardi]

Thanks for creating this PR and the great work already done on trying to support ICON data in this library!

Something to note is that if this is added in as is that people may assume this library already supports ICON data but without some normalisation constants added and ICON listed as an NWP provider here creating samples from it won't work, so my suggestion is that either this is added in this PR, or in a subsequent PR or this outstanding work is clearly documented in a Github issue or README, thanks!

I can compute the std and mean constants, do you have a script to do this for others NWP? How large of a sample do you take?

[Sukh-P]

I can compute the std and mean constants, do you have a script to do this for others NWP? How large of a sample do you take?

Thanks, that would be great! So I don't think we have a script in Github currently so just created this gist to share some example code of how I have calculated some of the normalisation stats previously, in the example I used 200 samples I think that would be fine for this too

[Sukh-P]

Hey @gabrielelibardi, hope you are doing well! Coming back to this one after a while and just wanted to check if you're planning on working on this still? If not maybe we can merge this in as is and then I can create another issue for someone to calculate the normalisation constants for icon-eu, thanks!

[gabrielelibardi]

Hey @Sukh-P hope you are also doing well! Still planning to work on this, but a bit busy at the moment with other stuff. You can also merge if nothing more is needed, up to you.

[Sukh-P]

Hey @Sukh-P hope you are also doing well! Still planning to work on this, but a bit busy at the moment with other stuff. You can also merge if nothing more is needed, up to you.

Okay great, so I have left a few comments which would be good to address, I don't think we need to add anything else apart from those to this PR, so after that we can merge this in.

There are some normalisation constants to be calculated as per my comments previously but I can create a separate issue for that, may need your input on how someone goes about fetching this data (as I believe you already fetched this from our HF) and then I can point to the gist I linked above for how someone can calculate these.

Thanks for the input!

[gabrielelibardi]

Hey @Sukh-P I made the changes you suggested, I also added some files to test the loading for icon (they are a bit heavy so we can remove those), also added the script to compute the mean and std_dev under utils, not sure if I should include it here or somewhere else or just leave it out.

[Sukh-P]

Hey @Sukh-P I made the changes you suggested, I also added some files to test the loading for icon (they are a bit heavy so we can remove those), also added the script to compute the mean and std_dev under utils, not sure if I should include it here or somewhere else or just leave it out.

Hey @gabrielelibardi, thanks for this, it looks good and almost there, on the test it may be a bit easier to create a small test zarr file for icon data on the fly as is done here https://github.com/openclimatefix/ocf-data-sampler/blob/main/tests/conftest.py#L92 for another NWP example, this way there are less files that need to be added to the repo and also easier to modify the test data in the future

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.40%. Comparing base (d34d49f) to head (5b91774).
Report is 30 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #72      +/-   ##
==========================================
+ Coverage   92.13%   93.40%   +1.27%     
==========================================
  Files          44       46       +2     
  Lines        1309     1350      +41     
==========================================
+ Hits         1206     1261      +55     
+ Misses        103       89      -14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[gabrielelibardi]

Hey @Sukh-P, makes sense, I just changed that so the test data is created on the fly. Let me know if there is anything else!

[Sukh-P]

All good I think! Just need to resolve some of the merge conflicts then should be good to go, thanks for all the work on this!

[gabrielelibardi]

there was a test failing once I rebased (test_copy_batch_to_device so I had to tweak it, let me know if it is ok @Sukh-P

[Sukh-P]

@gabrielelibardi any chance you could try removing that change to the test just so I can see the CI tests run to see if that particular tests fails? We just updated our CI workflow so they weren't running previously, sorry

[gabrielelibardi]

@Sukh-P sure, I reverted it now

[gabrielelibardi]

@Sukh-P btw I did not include all the multi level variables for the icon eu model, I saw that these are also not always included in the other nwps. If I include them I would say that we need to compute the mean and std_var for each level separately, but then again higher levels are not really that useful for pv generation forecast. So is it ok if for now only a subset of the variables is included?

[gabrielelibardi]

@Sukh-P could you share what commands from the nwp-consumer exactly do you run to download the icon-eu data you put on hugging face. I ran it to download the full parameters and somehow I don't get the same result as what I download from hugging face. Opened a related issue here openclimatefix/nwp-consumer#240

[Sukh-P]

@gabrielelibardi I think this is ready to merge in! I feel like the test should be okay because I have seen it run successfully before, if it fails after merging I will update it!

@Sukh-P btw I did not include all the multi level variables for the icon eu model, I saw that these are also not always included in the other nwps. If I include them I would say that we need to compute the mean and std_var for each level separately, but then again higher levels are not really that useful for pv generation forecast. So is it ok if for now only a subset of the variables is included?

This makes sense to me, we usually just care about surface/ground level variables anyway for PV forecasting so all good, thank you!

@Sukh-P could you share what commands from the nwp-consumer exactly do you run to download the icon-eu data you put on hugging face. I ran it to download the full parameters and somehow I don't get the same result as what I download from hugging face. Opened a related issue here openclimatefix/nwp-consumer#240

I think @devsjc will get back to you on this on that issue you raised, thanks for opening it!