Improving open_nwp function and dev-dependency change in pyproject.toml

[siddharth7113]

Pull Request

Description

While going through the code base I found in nwp.py the function open_nwp has an if-else block which requires changing everytime new provider is added, I modifed logic outside of the function to update in a private dictionaries, instead of adding if-else.

While installing dev dependency, I observed that these were grouped under [dependency-groups] , I changed it to [project.optional-dependencies] to make the uv pip install -e .[dev] work. (If this was intentional, I can revert the changes.) (Reverted due to CI breaking)

Fixes #

How Has This Been Tested?

All tests passes, except one unrelated test being skipped test_load_gfs

I haven't run black or ruff since on running black , it is reformatting more than 50 files, there is no defined pre-commit

• Yes

If your changes affect data processing, have you plotted any changes? i.e. have you done a quick sanity check?

• 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

[siddharth7113]

I have reverted pyproject.toml due to CI breaking, but would highly recommend to make pyproject have an optional dependency group as dev to make pip install -e .[dev] work.

[AUdaltsova]

Hi, great to see you around & good spot! Really nice bit of type hinting there. Left a couple of comments but good to go otherwise I think, thanks!

[siddharth7113]

Hi, great to see you around & good spot! Really nice bit of type hinting there. Left a couple of comments but good to go otherwise I think, thanks!

Hi @AUdaltsova , Thanks for the review, I have made the necessary changes, this should be good now !

[Sukh-P]

Hey both, just wanted to bring this comment/issue linked in it to your attention #379 (comment) which is related to a refactoring idea for this file and moving away from having unique loaders for each NWP since in fact for at least those in the same coord system like lat long they could be handled by a more generic loader, not saying this should block this change but maybe something to think about for the next change related to this, thanks

[AUdaltsova]

Hi @Sukh-P, yeah I thought about that but I don't think it's being actively worked on right now/is ready to be implemented any time soon (maybe I'm wrong!), so I don't think there's any harm in this being merged in the meantime as the work is done already anyway. Lmk if you disagree!

[Sukh-P]

So I agree that this shouldn't block this but I think this further change which can be implemented now basically if someone has capacity to pick it up (@siddharth7113 might be hinting in your direction here ha) would probably have a larger impact in terms of simplifying and reducing lines of code

[AUdaltsova]

Oh ok I see! @siddharth7113 would you be interested in taking up that issue? I think that would require putting in some renaming logic (different NWP sources might have different coordinate names that we rename into the same things), but then the lat-lon NWPs go through the same logic which can be shared between them, I think @Sukh-P recons 3-4 loaders can be removed after this.

Let me know if you'd like to take this up!

[siddharth7113]

So I agree that this shouldn't block this but I think this further change which can be implemented now basically if someone has capacity to pick it up (@siddharth7113 might be hinting in your direction here ha) would probably have a larger impact in terms of simplifying and reducing lines of code

Oh ok I see! @siddharth7113 would you be interested in taking up that issue? I think that would require putting in some renaming logic (different NWP sources might have different coordinate names that we rename into the same things), but then the lat-lon NWPs go through the same logic which can be shared between them, I think @Sukh-P recons 3-4 loaders can be removed after this.

Let me know if you'd like to take this up!

Hey @Sukh-P @AUdaltsova , Yeah I could work on that, I am trying to understand how ocf-data-sampler works, I think it would be a step in that direction, but Should I do it in a separate PR or this one itself?

[AUdaltsova]

Hi @siddharth7113, amazing, thanks for taking this up! Yeah maybe for cleanliness would be better to have a separate PR, and this one we would close for now as the new one should supersede it. Thanks again & reach out if you have questions!

[Sukh-P]

Hi @siddharth7113 I spoke to @AUdaltsova and I realised I muddied the water with my interjection and confused things, we decided that this PR can be merged in (I'll give it a quick review again after your latest changes) and then the additional changes I suggested if you're happy to work on that can be added on top of this in a separate PR, this way the PRs can be kept small, I can add you to that issue and give you more direction as well if you want it, hope that's alright, apologies all for the confusion!

[Sukh-P]

Thanks for this addition!

[Sukh-P]

@siddharth7113 will merge this in and tag you on the other issue related to these loader files, thanks!

[siddharth7113]

Hi @siddharth7113 I spoke to @AUdaltsova and I realised I muddied the water with my interjection and confused things, we decided that this PR can be merged in (I'll give it a quick review again after your latest changes) and then the additional changes I suggested if you're happy to work on that can be added on top of this in a separate PR, this way the PRs can be kept small, I can add you to that issue and give you more direction as well if you want it, hope that's alright, apologies all for the confusion!

No worries @Sukh-P , Thanks for merging this one, I 'll start working on the other issue soon !