-
Notifications
You must be signed in to change notification settings - Fork 80
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
Allow for small GRIB files #451
Conversation
Would you like to contribute one of your minimalist grib files for testing? We don't need to actually load the >200MB of coordinate grid, just verify that we can generate the one reference. |
It may take some time. I believe the file we have is technically proprietary, but I'll try to generate a very small GRIB file for testing. |
OK; I don't think the change here is contraversions, so happy to accept it without too. |
I cannot find a public GRIB file that is small for this test. I'm sorry. And I can't seem to figure out how to create one that is very small. (Have no idea how this one was created!) |
OK :) For the failing test, can you see if this is passing for you locally? Was there a new version of cfgrib or something? |
I can reproduce the error locally. At the moment, I'm not sure the cause. Seems unrelated to the changes I made in this PR, but I'll see if I can track it down. |
This test failed when I run in the |
The failing test also has nothing to do with GRIB. So, it's probably not a |
It is failing with the latest version of Xarray: 2024.3.0. If you downgrade to 2024.2.0, the tests pass. |
Bahh. I wonder if then the assert is too strict. It might, then, have something to do with pandas version too, since their time types changed within the last year. |
I can verify that if you comment out the |
I can make that change in the test if that's acceptable. |
perhaps |
Done! |
kerchunk/tests/test_combine.py
Outdated
@@ -730,7 +730,7 @@ def test_cftimes_to_normal(refs): | |||
engine="zarr", | |||
chunks={}, | |||
) | |||
assert z.time.dtype == "M8[s]" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what coo_dtypes
does, but Xarray definitely normalizes everything to ns
. Perhaps a bug was inadvertently fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I'm not sure what coo_dtypes
does, either, but it seems to no longer be doing what is expected with the latest version of Xarray.
I will have to return to this later (maybe tomorrow)! |
I'm back to this now, and I have figured out how to manufacture a special tiny GRIB2 file for testing, with nothing proprietary in it. So, I've added a test for this PR. There are still failing tests which have to do to updates/changes in other dependencies that will be dealt with in PR #450. So, I'll get back to this. |
If #450 is merged, I believe that all of the tests (including the newly added one for this PR) should pass or be skipped as expected. |
You'll need to merge from main I think |
Yay! |
This addresses issue #448.
closes #448