Skip to content

Add rotated_ll to supported CF CRS grid types - #459

Merged
blaylockbk merged 8 commits into
blaylockbk:mainfrom
nshankar:458-rotated-ll-proj
Aug 20, 2025
Merged

Add rotated_ll to supported CF CRS grid types#459
blaylockbk merged 8 commits into
blaylockbk:mainfrom
nshankar:458-rotated-ll-proj

Conversation

@nshankar

@nshankar nshankar commented Jul 5, 2025

Copy link
Copy Markdown
Contributor

Should solve #458 and gives a matching result to pygrib in the notebook.

⚠️: Not 100% sure about the following mapping, it works for HRDPS but it's unclear to me whether it's correct in general

projparams["lon_0"] = da.GRIB_longitudeOfSouthernPoleInDegrees
projparams["o_lon_p"] = 0.0
projparams["o_lat_p"] = -da.GRIB_latitudeOfSouthernPoleInDegrees

⚠️: I noticed two of the models in the notebook are failing, should that be a separate issue?

📝: I also added jupyter and pygrib as dev dependencies but that should be easy to remove if unwanted.

@nshankar nshankar changed the title Add rotated_ll grid type to CF CRS Add rotated_ll to supported CF CRS grid types Jul 5, 2025
@blaylockbk

Copy link
Copy Markdown
Owner

Thanks for the PR! I really appreciate it. Sorry it's taken some time to get to. I'd like to try this out myself before merging. Or, would you mind adding a test for this case?

I also added jupyter and pygrib as dev dependencies but that should be easy to remove if unwanted.
Hmm. I'm still thinking about this. I'd prefer pygrib to be an extra dependency not in dev (pygrib has had some issues in the past, and I'd prefer not requireing it in my dev environment). But I could be convinced otherwise. For Jupyter, I'm a VS Code user, and only need ipykernel. I'm assuming you're using JupyterLab. I'm probably fine with putting this in dev dependencies for other JupyterLab users.

@blaylockbk

Copy link
Copy Markdown
Owner

Not 100% sure about the following mapping, it works for HRDPS but it's unclear to me whether it's correct in general

I'm not aware of any other rotated_ll models asside from HRDPS, or is there?

@blaylockbk

Copy link
Copy Markdown
Owner

I noticed two of the models in the notebook are failing, should that be a separate issue?

This is probably due to not updating all my notebooks at each release. It could be a separate issue if you don't want to fix them in this PR.

@blaylockbk blaylockbk self-assigned this Aug 9, 2025
@blaylockbk
blaylockbk self-requested a review August 9, 2025 15:36
@blaylockbk blaylockbk added the bug Something isn't working label Aug 9, 2025
@blaylockbk blaylockbk added this to the Next Release milestone Aug 9, 2025
@nshankar

Copy link
Copy Markdown
Contributor Author

would you mind adding a test for this case?

Added a couple tests based on the structure you have set up. Please take a careful look.

Hmm. I'm still thinking about this. I'd prefer pygrib to be an extra dependency not in dev

Agree pygrib is sometimes problematic. I can move everything to extras, is that where you normally put testing dependencies? I had to add netcdf into dev as well in order get my tests passing locally (running uv run pytest).

It could be a separate issue if you don't want to fix them in this PR.

Yes can we track that separately please?

Comment thread tests/test_hrdps.py
from herbie import Herbie, config

now = datetime.now()
latest = pd.Timestamp("now").floor("6h") - pd.Timedelta("6h")

@nshankar nshankar Aug 10, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

DRAGONS:

At approx midnight UTC the HRDPS servers get cleared of the previous day's forecasts. However there is no new forecast available yet until 06Z. During that time these tests will fail.

What do you think is the right way to handle this in the Herbie package?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added this info to the module docstring

Comment thread pyproject.toml Outdated
Comment thread pyproject.toml Outdated
@mattw-nws

Copy link
Copy Markdown

Not 100% sure about the following mapping, it works for HRDPS but it's unclear to me whether it's correct in general

I'm not aware of any other rotated_ll models asside from HRDPS, or is there?

I'm hitting this when trying to monkey-patch a fix for the RRFS plugin... so seems to be in use there.

@mattw-nws

Copy link
Copy Markdown

I'm hitting this when trying to monkey-patch a fix for the RRFS plugin... so seems to be in use there.

I should specify, I think this is specific to the full-domain RRFS's projection (nat product). Adding this change fixed it--at least when pulling a COG into QGIS (having trouble with OpenLayers/proj4js with that projection so I dumped it for now and went back to CONUS domain).

@nshankar

Copy link
Copy Markdown
Contributor Author

I see some tests are failing because of pygrib, however that dependency is needed to run the notebook. Is there another dependency group I can put it in @blaylockbk?

@blaylockbk

Copy link
Copy Markdown
Owner

What if we just kept it out of a dependency group and left it only in an extra dependency by itself.

@blaylockbk

Copy link
Copy Markdown
Owner

Pygrib is already an optional dependency. Maybe just keep it there and not in a group for now.

@nshankar

Copy link
Copy Markdown
Contributor Author

Done!

@nshankar

Copy link
Copy Markdown
Contributor Author

I followed the steps here https://github.com/blaylockbk/Herbie/blob/main/.github/workflows/tests-python.yml

uv sync --extra extras
uv run pytest

and the tests I've added are passing!

@blaylockbk

Copy link
Copy Markdown
Owner

Great! I think the error on Windows is related to cfgrib and eccodes. This can be addressed in a separate issue. The rest is good to merge. Thanks for this contribution. I really appreciate it. 😃

@blaylockbk
blaylockbk merged commit f25ca7a into blaylockbk:main Aug 20, 2025
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants