BAMWelDX / weldx

The welding data exchange format
https://www.bam.de/weldx
BSD 3-Clause "New" or "Revised" License
19 stars 9 forks source link

Fix XArray LCS join #861

Closed marscher closed 1 year ago

marscher commented 1 year ago

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

github-actions[bot] commented 1 year ago

Test Results

2 187 tests  ±0   2 186 :heavy_check_mark: ±0   2m 39s :stopwatch: +8s        1 suites ±0          1 :zzz: ±0         1 files   ±0          0 :x: ±0 

Results for commit 2e0ea22b. ± Comparison against base commit 4215eb04.

:recycle: This comment has been updated with latest results.

CagtayFabry commented 1 year ago

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 commented 1 year ago

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

CagtayFabry commented 1 year ago

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 commented 1 year ago

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

CagtayFabry commented 1 year ago

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

marscher commented 1 year ago

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 commented 1 year ago

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

codecov[bot] commented 1 year ago

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%) :arrow_up:

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

marscher commented 1 year ago

Brilliant, then let's give a shot!