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

Fix XArray LCS join #861

Merged
merged 23 commits into from
Apr 3, 2023
Merged

Conversation

marscher
Copy link
Contributor

Changes

Some new version of XArray changed the merge logic in a way being incompatible with our demands in LCS. Setting the join method of the merge to "override" seems to work (at least the tests are passing).

Related Issues

Properly fixes #810

Checks

  • updated CHANGELOG.rst

pyproject.toml Outdated
@@ -38,7 +38,7 @@ dependencies = [
"numpy >=1.20",
"asdf >=2.8.2,!=2.14.0,!=2.14.1,!=2.14.2",
"pandas >=1.0",
"xarray >=0.19,<2022.09.0",
"xarray >=2023.3.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually we can also think of finding the XArray version which introduced Dataset.merge(join="override"), but I found it easier to just request at least a recent version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This XArray version dropped support for Python 3.8!
As we are at 3.11 now and 3.12 pending, we should do the same.

@github-actions
Copy link

github-actions bot commented Mar 27, 2023

Test Results

2 187 tests  ±0   2 186 ✔️ ±0   2m 39s ⏱️ +8s
       1 suites ±0          1 💤 ±0 
       1 files   ±0          0 ±0 

Results for commit 2e0ea22. ± Comparison against base commit 4215eb0.

♻️ This comment has been updated with latest results.

@CagtayFabry
Copy link
Member

from the description of xr.merge(join="override") I don't think this is what we actually want since non equal indexes seem to be overridden.
I am not sure if there ever was any test that would catch this

@marscher
Copy link
Contributor Author

At least we have some test cases which produce non equal indices. Otherwise it would not fail with join="equal", right?

@CagtayFabry
Copy link
Member

At least we have some test cases which produce non equal indices. Otherwise it would not fail with join="equal", right?

I assume that's the case, but it would be good to check it properly
Did you test if this runs on the original broken xarray version 2022.9.0?

@marscher
Copy link
Contributor Author

marscher commented Apr 3, 2023

I just ran the test now with xarray 2022.09.0 and it works as expected.

@CagtayFabry
Copy link
Member

I just ran the test now with xarray 2022.09.0 and it works as expected.

great, thank you
I will try to take a look into the PR later and we can just pin >=2022.09.0

@CagtayFabry CagtayFabry self-requested a review April 3, 2023 09:41
@marscher
Copy link
Contributor Author

marscher commented Apr 3, 2023

Python 3.11 currently fails with

   error    libmamba Could not solve for environment specs
      The following packages are incompatible
      ├─ python 3.11**  is requested and can be installed;
      ├─ weldx_widgets >=0.2  is installable and it requires
      │  └─ weldx [>=0.5 |>=0.6 ] with the potential options
      │     ├─ weldx [0.5.0|0.5.1|0.5.2] would require
      │     │  └─ python >=3.8,<3.10 , which conflicts with any installable versions previously reported;
      │     ├─ weldx [0.6.0|0.6.1|0.6.2] would require
      │     │  └─ python >=3.8,<3.11 , which conflicts with any installable versions previously reported;
      │     └─ weldx [0.6.3|0.6.4] would require
      │        └─ xarray >=0.19,<2022.09.0 , which can be installed;
      └─ xarray >=2022.9.0  is uninstallable because it conflicts with any installable versions previously reported.

So we would need to release another version to fix this for Python 3.11, but I do not think this is urgent.

@CagtayFabry
Copy link
Member

I think I caught the bug though I am not completely sure what caused the change in xarray behavior (I think improved attribute propagation in xarray functions)

I don't mind releasing 0.6.5 at this point, it's nice to be back on the xarray release cycle

@CagtayFabry CagtayFabry added bug Something isn't working xarray issues related to handling xarray objects dependencies changes in package dependencies labels Apr 3, 2023
@codecov
Copy link

codecov bot commented Apr 3, 2023

Codecov Report

Merging #861 (2e0ea22) into master (4215eb0) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #861   +/-   ##
=======================================
  Coverage   96.47%   96.47%           
=======================================
  Files          95       95           
  Lines        6235     6238    +3     
=======================================
+ Hits         6015     6018    +3     
  Misses        220      220           
Impacted Files Coverage Δ
weldx/util/xarray.py 95.80% <100.00%> (+0.04%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@marscher
Copy link
Contributor Author

marscher commented Apr 3, 2023

Brilliant, then let's give a shot!

@CagtayFabry CagtayFabry merged commit ae16d42 into BAMWelDX:master Apr 3, 2023
@marscher marscher deleted the fix_xarray_lcs_join branch April 3, 2023 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working dependencies changes in package dependencies xarray issues related to handling xarray objects
Projects
None yet
Development

Successfully merging this pull request may close these issues.

xarray-2022.9.0 breaks weldx
2 participants