-
Notifications
You must be signed in to change notification settings - Fork 24
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
Ab/filters dtype #66
Ab/filters dtype #66
Conversation
pyproject.toml
Outdated
@@ -21,7 +21,7 @@ classifiers = [ | |||
requires-python = ">=3.9" | |||
dynamic = ["version"] | |||
dependencies = [ | |||
"xarray", | |||
"xarray@git+https://github.com/TomNicholas/xarray#egg=concat-no-indexes", |
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.
This was required to get a few tests to pass. Even with this change I am still getting the error
E NotImplementedError: ManifestArrays can't be converted into numpy arrays or pandas Index objects
from virtualizarr/tests/test_xarray.py:175 TestConcat.test_concat_dim_coords_along_existing_dim
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.
The CI in this PR is not using the correct branch of xarray - the CI shows the error is raised on this line
result = type(datasets[0])(result_vars, attrs=result_attrs)
but in pydata/xarray#8872 and my concat-no-indexes
branch this line is different
the line should read
result = type(datasets[0])(result_vars, coords=coords, attrs=result_attrs)
so something is up with the pip-installing of that specific branch.
(I'm trying to get pydata/xarray#8872 merged so that we can just use xarray main
)
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.
Also note that this test passes on the CI on main
of this package
https://github.com/TomNicholas/VirtualiZarr/actions/runs/8457581269/job/23169875291
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.
ok trying with the default xarray branch, like in the main branch of this package.
Thanks for the issue and for the fix @abarciauskas-bgse ! I think this PR is mergeable, but the tests are failing because the wrong version of xarray is being installed by the CI... |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #66 +/- ##
==========================================
+ Coverage 90.29% 90.39% +0.09%
==========================================
Files 14 14
Lines 938 947 +9
==========================================
+ Hits 847 856 +9
Misses 91 91
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Tests are now passing @TomNicholas, let me know if you have other comments 👍🏽 |
This looks great, thank you! |
Addresses #65