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

Allow for small GRIB files #451

Merged
merged 7 commits into from
Apr 12, 2024
Merged

Conversation

kmpaul
Copy link
Contributor

@kmpaul kmpaul commented Apr 11, 2024

This addresses issue #448.

closes #448

@martindurant
Copy link
Member

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.

@kmpaul
Copy link
Contributor Author

kmpaul commented Apr 11, 2024

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.

@martindurant
Copy link
Member

OK; I don't think the change here is contraversions, so happy to accept it without too.

@kmpaul
Copy link
Contributor Author

kmpaul commented Apr 11, 2024

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!)

@martindurant
Copy link
Member

OK :)

For the failing test, can you see if this is passing for you locally? Was there a new version of cfgrib or something?

@kmpaul
Copy link
Contributor Author

kmpaul commented Apr 11, 2024

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.

@kmpaul
Copy link
Contributor Author

kmpaul commented Apr 11, 2024

This test failed when I run in the main branch, too. I'll see if I can figure out what is causing the change.

@kmpaul
Copy link
Contributor Author

kmpaul commented Apr 11, 2024

The failing test also has nothing to do with GRIB. So, it's probably not a cfgrib version.

@kmpaul
Copy link
Contributor Author

kmpaul commented Apr 11, 2024

It is failing with the latest version of Xarray: 2024.3.0. If you downgrade to 2024.2.0, the tests pass.

@martindurant
Copy link
Member

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.
Perhaps we shouldn't check the type at all, just make sure the values are right.

@kmpaul
Copy link
Contributor Author

kmpaul commented Apr 11, 2024

I can verify that if you comment out the dtype check, the value check passes.

@kmpaul
Copy link
Contributor Author

kmpaul commented Apr 11, 2024

I can make that change in the test if that's acceptable.

@martindurant
Copy link
Member

perhaps dtype.kind == "M" is still valuable

@kmpaul
Copy link
Contributor Author

kmpaul commented Apr 11, 2024

Done!

@@ -730,7 +730,7 @@ def test_cftimes_to_normal(refs):
engine="zarr",
chunks={},
)
assert z.time.dtype == "M8[s]"
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@kmpaul
Copy link
Contributor Author

kmpaul commented Apr 11, 2024

I will have to return to this later (maybe tomorrow)!

@kmpaul
Copy link
Contributor Author

kmpaul commented Apr 12, 2024

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.

@kmpaul kmpaul mentioned this pull request Apr 12, 2024
@kmpaul
Copy link
Contributor Author

kmpaul commented Apr 12, 2024

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.

@martindurant
Copy link
Member

You'll need to merge from main I think

@kmpaul
Copy link
Contributor Author

kmpaul commented Apr 12, 2024

Yay!

@martindurant martindurant merged commit dee5938 into fsspec:main Apr 12, 2024
5 checks passed
@kmpaul kmpaul deleted the kpaul/issue-448 branch April 12, 2024 14:13
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.

Support for small files in _split_file
3 participants