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

Ab/filters dtype #66

Merged
merged 3 commits into from
Mar 29, 2024
Merged

Ab/filters dtype #66

merged 3 commits into from
Mar 29, 2024

Conversation

abarciauskas-bgse
Copy link
Collaborator

Addresses #65

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",
Copy link
Collaborator Author

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

Copy link
Collaborator

@TomNicholas TomNicholas Mar 29, 2024

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)

Copy link
Collaborator

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

Copy link
Collaborator Author

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.

@TomNicholas
Copy link
Collaborator

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...

Copy link

codecov bot commented Mar 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.39%. Comparing base (2c5be3f) to head (e36b516).

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              
Flag Coverage Δ
unittests 90.39% <100.00%> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@abarciauskas-bgse
Copy link
Collaborator Author

Tests are now passing @TomNicholas, let me know if you have other comments 👍🏽

@TomNicholas
Copy link
Collaborator

This looks great, thank you!

@TomNicholas TomNicholas merged commit aa18eaa into main Mar 29, 2024
8 checks passed
@TomNicholas TomNicholas deleted the ab/filters-dtype branch March 29, 2024 20:45
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