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

Add test for appending with cftime #414

Merged
merged 9 commits into from
Feb 2, 2024
Merged

Conversation

maxrjones
Copy link
Contributor

No description provided.

@martindurant
Copy link
Member

This is probably a bad choice of unit

"units": "seconds since 1970-01-01T00:00:00"

because that's also the standard UNIX definition, so having no units would have the same effect.

A nice concise error:

numpy.core._exceptions._UFuncNoLoopError: ufunc 'less' did not contain a loop with signature matching types (<class 'numpy.dtypes.Int64DType'>, <class 'numpy.dtypes.DateTime64DType'>) -> None

I'll look into the error, though, I would have expected this to work.

@martindurant
Copy link
Member

OK, so of course xarray converts times automatically unless told otherwise.

(also fix drop cords in kerchunk engine)
@maxrjones
Copy link
Contributor Author

Thanks for the fix @martindurant! Did you not like the drop_vars option in the xarray backend?

Also, a bit unrelated but do you mind sharing with me what ... passed to __getitem__ does for zarr (referencing the following)?

o = z[selector.split(":", 1)[1]][...]

@martindurant
Copy link
Member

Did you not like the drop_vars option in the xarray backend?

It was not being applied after the dataset was already loaded. This does the right thing for a normal load, but if some array fails to load (as I found with some time conversion), the options needs to be in the first open_dataset call. So you can use the argument as before, but it should work better now :)

what ... passed to getitem does

It would be the same as [:] (i.e., grab the actual values as an array) but it also works for zero-dimension arrays.

@maxrjones
Copy link
Contributor Author

Did you not like the drop_vars option in the xarray backend?

It was not being applied after the dataset was already loaded. This does the right thing for a normal load, but if some array fails to load (as I found with some time conversion), the options needs to be in the first open_dataset call. So you can use the argument as before, but it should work better now :)

what ... passed to getitem does

It would be the same as [:] (i.e., grab the actual values as an array) but it also works for zero-dimension arrays.

Thanks for the explanations and your work on the append feature!

@maxrjones
Copy link
Contributor Author

The kerchunk tests take a long time to run in the CI, and it looks like most of the time is spent on the setup-micromamba step. Would you be open to enabling caching in setup-micromamba to speed up the workflows?

@martindurant
Copy link
Member

Certainly would be happy to have caching. Actually, conda now uses the mamba solver, so it might be faster again - I don't mind, so long as it works!

@martindurant
Copy link
Member

I used dict | syntax, which is py>=3.9; updated versions. Should probably update required python version generally.

@martindurant
Copy link
Member

@maxrjones , env building on py39 seems stuck, any idea why?

@maxrjones
Copy link
Contributor Author

@maxrjones , env building on py39 seems stuck, any idea why?

I pushed a couple fixes for the env errors

@martindurant martindurant merged commit 23c1146 into fsspec:main Feb 2, 2024
5 checks passed
@maxrjones maxrjones deleted the append-time branch February 2, 2024 18:37
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.

2 participants