Add xarray support to `MathematicalExpression` #621

vhirtham · 2021-10-29T12:26:48Z

Changes

MathematicalExpression now uses xarray internally. This can simplify multi-dimensional expressions if one uses DataArrays with correctly named dimensions. If no DataArrays are used as parameters and variables, the behavior should remain more or less the same since xarray will automatically add dimension names.

In the current version evaluate will now return a DataArray instead of a quantity. This requires some changes to the internals of the TimeSeries. However, when those changes are fully applied, the TimeSeries implementation should be much simpler than before.

Related Issues

Closes #619

Checks

github-actions · 2021-10-29T12:32:41Z

Unit Test Results

      1 files ±0       1 suites ±0 2m 4s ⏱️ -38s
1 925 tests +2 1 925 ✔️ +2 0 💤 ±0 0 ❌ ±0

Results for commit e2f0e39. ± Comparison against base commit 6e78782.

♻️ This comment has been updated with latest results.

weldx/core.py

review-notebook-app · 2021-11-01T12:53:05Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

codecov · 2021-11-01T13:01:16Z

Codecov Report

Merging #621 (e2f0e39) into master (6e78782) will decrease coverage by 0.06%.
The diff coverage is 94.54%.

@@            Coverage Diff             @@
##           master     #621      +/-   ##
==========================================
- Coverage   94.62%   94.56%   -0.07%     
==========================================
  Files          93       93              
  Lines        5936     5958      +22     
==========================================
+ Hits         5617     5634      +17     
- Misses        319      324       +5

Impacted Files	Coverage Δ
weldx/time.py	`97.73% <80.00%> (-0.43%)`	⬇️
weldx/tags/core/mathematical_expression.py	`96.66% <95.00%> (-3.34%)`	⬇️
weldx/core.py	`97.35% <96.55%> (-1.31%)`	⬇️
weldx/measurement.py	`95.57% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e78782...e2f0e39. Read the comment docs.

weldx/core.py

weldx/tests/test_core.py

# Conflicts: # weldx/time.py

vhirtham · 2021-11-02T10:05:56Z

I updated the schema and did some small refactoring. @CagtayFabry You might want to have a look at the changes. If there are no further concerns, I will merge this PR after lunch.

CagtayFabry · 2021-11-02T10:06:46Z

weldx/schemas/weldx.bam.de/weldx/core/mathematical_expression-0.1.0.yaml

+    anyOf:
+      - wx_property_tag: "asdf://weldx.bam.de/weldx/tags/core/data_array-0.1.*"
+      - wx_property_tag: "asdf://weldx.bam.de/weldx/tags/units/quantity-0.1.*"


I would prefer if we don't have data_array here as it is very verbose, that should still be possible right? @vhirtham

Well, the solution would be to not use data arrays as parameters if you don't need to. But if I have a multi-dimensional equation that uses data arrays as parameters to make my life easier, I can't store it in asdf unless I sort out the dimensions myself and restore the parameters as quantities.

CagtayFabry · 2021-11-02T10:08:38Z

I updated the schema and did some small refactoring. @CagtayFabry You might want to have a look at the changes. If there are no further concerns, I will merge this PR after lunch.

if we want to allow data_array tags in the schema we should bump the schema version (which I would like to avoid for the 0.5.x releases)

vhirtham · 2021-11-02T10:18:59Z

I updated the schema and did some small refactoring. @CagtayFabry You might want to have a look at the changes. If there are no further concerns, I will merge this PR after lunch.

if we want to allow data_array tags in the schema we should bump the schema version (which I would like to avoid for the 0.5.x releases)

Since the schema is now less restrictive, no existing file will break. Furthermore, the current userbase is just us. So I think we can get away with not bumping the version.

CagtayFabry · 2021-11-02T10:34:14Z

I updated the schema and did some small refactoring. @CagtayFabry You might want to have a look at the changes. If there are no further concerns, I will merge this PR after lunch.

if we want to allow data_array tags in the schema we should bump the schema version (which I would like to avoid for the 0.5.x releases)

Since the schema is now less restrictive, no existing file will break. Furthermore, the current userbase is just us. So I think we can get away with not bumping the version.

true, but I find the xarray schema for simple things like this borderline unreadable in the schema
Also I am not sure how far the implementation for wx_unit validation is with xarray which I figure would be very important here

can some parameters be passed as xarray and others as quantities in the current python implementation?

CagtayFabry · 2021-11-02T10:48:32Z

weldx/core.py

@@ -450,7 +467,7 @@ def _interp_time_discrete(self, time: Time) -> xr.DataArray:
        """Interpolate the time series if its data is composed of discrete values."""
        return ut.xr_interp_like(
            self._data,
-            {"time": time.as_timedelta()},
+            {"time": time.as_timedelta_index()},


we should pass the reference time here as well

well, if you use as_data_array here, you get the "non-monotonic" error I mentioned above 😉

vhirtham · 2021-11-02T11:07:52Z

I updated the schema and did some small refactoring. @CagtayFabry You might want to have a look at the changes. If there are no further concerns, I will merge this PR after lunch.

if we want to allow data_array tags in the schema we should bump the schema version (which I would like to avoid for the 0.5.x releases)

Since the schema is now less restrictive, no existing file will break. Furthermore, the current userbase is just us. So I think we can get away with not bumping the version.

true, but I find the xarray schema for simple things like this borderline unreadable in the schema

Why in the schema? Did you mean the written file?
My opinion: The goal should always be to get full functionality with good readability. However, if we can't have both, then functionality is way more important to me.

Also I am not sure how far the implementation for wx_unit validation is with xarray which I figure would be very important here

If this is a problem it needs to be addressed in a follow-up issue/PR.

can some parameters be passed as xarray and others as quantities in the current python implementation?

Yes. Every parameter is stored internally (and also written to asdf) as a quantity unless it is a DataArray, which will be stored as DataArray. Only during evaluation, everything is promoted to a DataArray to perform the calculation. But that doesn't affect the internal variables

CagtayFabry · 2021-11-02T11:14:35Z

Yes. Every parameter is stored internally (and also written to asdf) as a quantity unless it is a DataArray, which will be stored as DataArray. Only during evaluation, everything is promoted to a DataArray to perform the calculation. But that doesn't affect the internal variables

In that case I guess the schema wouldn't work for 'mixed' parameters anyway since wx_property_tag doesn't support this

I'll catch up with you later so we can discuss in detail :)

vhirtham · 2021-11-02T11:52:21Z

Yes. Every parameter is stored internally (and also written to asdf) as a quantity unless it is a DataArray, which will be stored as DataArray. Only during evaluation, everything is promoted to a DataArray to perform the calculation. But that doesn't affect the internal variables

In that case I guess the schema wouldn't work for 'mixed' parameters anyway since wx_property_tag doesn't support this

I'll catch up with you later so we can discuss in detail :)

All right, I will take care of the tutorials in the meantime.

vhirtham · 2021-11-02T16:30:07Z

Just implemented the matadata approach and tested it. Works like a charm. I will add the tuple construction tomorrow and add a warning if coordinates are dropped.

…_xarray

CagtayFabry

looks really clean now 👍

vhirtham added 3 commits October 29, 2021 11:34

Add set_parameters and optimize code

7368ee5

Start updating TimeSeries

4c339cb

Uncomment tests

361fedf

vhirtham added xarray issues related to handling xarray objects core weldx core classes and functions labels Oct 29, 2021

vhirtham self-assigned this Oct 29, 2021

CagtayFabry reviewed Nov 1, 2021

View reviewed changes

weldx/core.py Outdated Show resolved Hide resolved

vhirtham added 6 commits November 1, 2021 10:23

Fix some tests

ac63523

Fix tests

0f003eb

Fix pre-commit

a6876dc

Merge branch 'master' into me_xarray

f7492a3

Fix notebooks

a1ca3d2

Fix mypy issue

ef09dc0

Fix docs mistake

38c16f7

vhirtham marked this pull request as ready for review November 1, 2021 15:31

CagtayFabry approved these changes Nov 2, 2021

View reviewed changes

weldx/core.py Outdated Show resolved Hide resolved

weldx/tests/test_core.py Show resolved Hide resolved

vhirtham and others added 5 commits November 2, 2021 09:08

Apply suggestion from PR

1c4675a

use timedelta values in Time.as_data_array by default

9e8bfca

Merge remote-tracking branch 'vhirtham/me_xarray' into me_xarray

e93144a

# Conflicts: # weldx/time.py

always use index type

1f005e1

Update changelog

ab145aa

CagtayFabry added this to the 0.5.1 milestone Nov 2, 2021

vhirtham added 3 commits November 2, 2021 10:55

Update scheme

d1eb634

Add some enhancements

d859ae9

Add blank line

1e03a83

CagtayFabry requested changes Nov 2, 2021

View reviewed changes

Merge branch 'master' into me_xarray

76e971e

CagtayFabry mentioned this pull request Nov 2, 2021

new release or patched pypi release #624

Closed

CagtayFabry reviewed Nov 2, 2021

View reviewed changes

vhirtham added 2 commits November 2, 2021 15:54

Restore old schema version

64d2a91

Test metadata dims

68a9555

vhirtham added 5 commits November 3, 2021 10:00

Merge branch 'master' into me_xarray

14109d4

Add tuple parameters

7df0893

Merge branch 'me_xarray' of https://github.com/vhirtham/weldx into me…

5926144

…_xarray

Fix mypy issue

45c8725

Add warning if coordinates are dropped

e2f0e39

vhirtham requested a review from CagtayFabry November 3, 2021 14:01

CagtayFabry approved these changes Nov 3, 2021

View reviewed changes

vhirtham merged commit d8dd276 into BAMWelDX:master Nov 3, 2021

vhirtham deleted the me_xarray branch November 3, 2021 14:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add xarray support to `MathematicalExpression` #621

Add xarray support to `MathematicalExpression` #621

vhirtham commented Oct 29, 2021 •

edited

Loading

github-actions bot commented Oct 29, 2021 •

edited

Loading

review-notebook-app bot commented Nov 1, 2021

codecov bot commented Nov 1, 2021 •

edited

Loading

vhirtham commented Nov 2, 2021

CagtayFabry Nov 2, 2021

vhirtham Nov 2, 2021

CagtayFabry commented Nov 2, 2021

vhirtham commented Nov 2, 2021

CagtayFabry commented Nov 2, 2021

CagtayFabry Nov 2, 2021

vhirtham Nov 2, 2021

vhirtham commented Nov 2, 2021

CagtayFabry commented Nov 2, 2021

vhirtham commented Nov 2, 2021

vhirtham commented Nov 2, 2021

CagtayFabry left a comment

Add xarray support to MathematicalExpression #621

Add xarray support to MathematicalExpression #621

Conversation

vhirtham commented Oct 29, 2021 • edited Loading

Changes

Related Issues

Checks

github-actions bot commented Oct 29, 2021 • edited Loading

Unit Test Results

review-notebook-app bot commented Nov 1, 2021

codecov bot commented Nov 1, 2021 • edited Loading

Codecov Report

vhirtham commented Nov 2, 2021

CagtayFabry Nov 2, 2021

Choose a reason for hiding this comment

vhirtham Nov 2, 2021

Choose a reason for hiding this comment

CagtayFabry commented Nov 2, 2021

vhirtham commented Nov 2, 2021

CagtayFabry commented Nov 2, 2021

CagtayFabry Nov 2, 2021

Choose a reason for hiding this comment

vhirtham Nov 2, 2021

Choose a reason for hiding this comment

vhirtham commented Nov 2, 2021

CagtayFabry commented Nov 2, 2021

vhirtham commented Nov 2, 2021

vhirtham commented Nov 2, 2021

CagtayFabry left a comment

Choose a reason for hiding this comment

Add xarray support to `MathematicalExpression` #621

Add xarray support to `MathematicalExpression` #621

vhirtham commented Oct 29, 2021 •

edited

Loading

github-actions bot commented Oct 29, 2021 •

edited

Loading

codecov bot commented Nov 1, 2021 •

edited

Loading