Check time coords #635

CagtayFabry · 2021-11-04T16:48:48Z

Changes

check for time in xarray coordinates instead of dimensions in some cases

In cases where we access the time coordinate value I think it makes sense to check for time in the coordinates instead of the array dimensions. If time is a dimension without coordinates we would only get integer values which would be error prone.

In contrast if we actually want to check if an object is time_dependent (changing in time) we should look for time in the dimensions

However so far it is a bit inconsistent with single timestamps outside of the definition range:

from weldx import LocalCoordinateSystem as LCS

lcs = LCS(coordinates=[[0,0,0],[1,0,0]],time=["0s","3s"])
lcs.interp_time("2s")
#> <LocalCoordinateSystem>
#> Dimensions:      (c: 3, v: 3)
#> Coordinates:
#>   * c            (c) <U1 'x' 'y' 'z'
#>   * v            (v) int32 0 1 2
#>     time         timedelta64[ns] 00:00:02
#> Data variables:
#>     orientation  (v, c) float64 1.0 0.0 0.0 0.0 1.0 0.0 0.0 0.0 1.0
#>     coordinates  (c) float64 0.6667 0.0 0.0

lcs.interp_time("4s")
#> <LocalCoordinateSystem>
#> Dimensions:      (c: 3, v: 3)
#> Coordinates:
#>   * c            (c) <U1 'x' 'y' 'z'
#>   * v            (v) int32 0 1 2
#> Data variables:
#>     orientation  (v, c) float64 1.0 0.0 0.0 0.0 1.0 0.0 0.0 0.0 1.0
#>     coordinates  (c) float64 1.0 0.0 0.0

Checks

updated CHANGELOG.rst
updated tests

github-actions · 2021-11-04T16:53:03Z

Unit Test Results

      1 files ±0       1 suites ±0 1m 57s ⏱️ +17s
1 930 tests ±0 1 930 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 97677cf. ± Comparison against base commit 299d199.

♻️ This comment has been updated with latest results.

…o xarray

codecov · 2021-11-05T11:50:02Z

Codecov Report

Merging #635 (97677cf) into master (299d199) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #635   +/-   ##
=======================================
  Coverage   94.49%   94.49%           
=======================================
  Files          93       93           
  Lines        6011     6014    +3     
=======================================
+ Hits         5680     5683    +3     
  Misses        331      331

Impacted Files	Coverage Δ
weldx/geometry.py	`97.49% <100.00%> (+<0.01%)`	⬆️
...gs/core/transformations/local_coordinate_system.py	`100.00% <100.00%> (ø)`
weldx/time.py	`97.75% <100.00%> (+0.02%)`	⬆️
weldx/transformations/cs_manager.py	`98.76% <100.00%> (ø)`
weldx/transformations/local_cs.py	`97.04% <100.00%> (ø)`
weldx/util/xarray.py	`98.38% <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 299d199...97677cf. Read the comment docs.

CagtayFabry · 2021-11-05T12:37:05Z

weldx/tests/transformations/test_local_cs.py

+    assert lcs_interp.is_time_dependent is False
+    assert lcs_interp.time.equals(Time("2s"))


this is one case where the updated behavior differs slightly

lcs_interp.is_time_dependent returns False but we can still access the (singular) timestamp from the LCS

CagtayFabry · 2021-11-05T12:37:25Z

weldx/time.py

+        if time.shape:
+            time_index = pd.Index(time.values)
+        else:
+            time_index = pd.Index([time.values])


singular values weren't really handled all that well before

CagtayFabry · 2021-11-05T12:38:43Z

weldx/transformations/local_cs.py

@@ -548,7 +549,7 @@ def is_time_dependent(self) -> bool:
            `True` if the coordinate system is time dependent, `False` otherwise.

        """
-        return self.time is not None or self._coord_ts is not None
+        return self._coord_ts is not None or ("time" in self._dataset.dims)


I think this is the most important change, a LCS can be time independent but still return a .time value

CagtayFabry · 2021-11-05T12:39:38Z

weldx/transformations/local_cs.py

@@ -686,9 +687,9 @@ def _interp_time_coordinates(self, time: Time) -> xr.DataArray:
        if "time" not in self.coordinates.dims:  # don't interpolate static
            return self.coordinates
        if time.max() <= self.time.min():  # only use edge timestamp
-            return self.coordinates[0]
+            return self.coordinates.isel(time=0).data


we could also skip the .data here but in that case we would have to decide which "edge" timestamp we want to assign, however this is now more consistent for coordinates and orientations

CagtayFabry · 2021-11-10T08:08:27Z

weldx/geometry.py

+        # make sure we have correct dimension order
+        self.coordinates = self.coordinates.transpose(..., "n", "c")
+


this was causing the troubles with scan data transformation as an incorrectly transposed xarray (p,c,n) was passed to SpatialData @vhirtham

Nice finding. I recently also passed a "misformed" xarray to ´SpatialData´ that gave me some headaches. The time coordinate was a datetime instead of a timedelta with an optional reference time. The error message was a bit cryptic and only occurred if time-dependent transformations were performed. The question is if we want to catch this too. While we might remember the format requirements, our new 2 users from the lab might get totally confused by such errors. Maybe we can introduce an xr_format_data_array function that orders dimensions, correct wrong time inputs and possibly other stuff.

can you reproduce this bug?

the time values can be formatted here by using the .weldx.time_ref_restore xarray accessor, however all operations that rely on these time formats (e.g. interpolation) should also make sure to handle these internally so it would be good to know where this happens.

either way we should also make use of check_coords more often

Ideally, one can grasp the expected format from the function inputs

import xarray as xr from weldx import CoordinateSystemManager, SpatialData, Time, WXRotation rot = WXRotation.from_euler("z", [0, 45, 90], True).as_matrix() csm = CoordinateSystemManager("root") csm.create_cs("workpiece", "root", coordinates=[0, 1, 2]) csm.create_cs( "TCP", "root", orientation=rot, coordinates=[[0, 0, 0], [1, 0, 0], [2, 0, 0]], time=Time(["2000", "2001", "2002"]), ) da = xr.DataArray( [[[0, -1, 1], [0, 1, 1]], [[0, -1, 2], [0, 1, 2]], [[0, -1, -1], [0, 1, -1]]], dims=["time", "n", "c"], coords={"time": Time(["2000", "2001", "2002"]).as_pandas_index()}, ) sd = SpatialData(da) csm.assign_data(sd, "data", "TCP", "workpiece")

This was the script I used to understand the error. I didn't create an issue cause I thought it was my own stupidity to violate the expected xarray format. However, it is probably better if those input violations are either handled or give an instant and clear response to the user as you said. The annoying thing here is: If you remover the instant transformation to the workpiece cs, the error happens later (for example during a .plot call or wherever the first transformation occurs) and getting the connection isn't that easy anymore.

CagtayFabry · 2021-11-10T08:21:28Z

added some minor changes but I think this is good now @vhirtham

CagtayFabry added 2 commits November 4, 2021 17:33

check for time in coords instead of dims

0040525

check time dependency on dims

4432cf1

CagtayFabry added the xarray issues related to handling xarray objects label Nov 4, 2021

CagtayFabry added 3 commits November 4, 2021 19:14

allow Time conversion from singular times, update LCS interpolation t…

2993b20

…o xarray

adjust cs_manager

f686adf

revert to old behavior

0e98a22

CagtayFabry added 2 commits November 5, 2021 12:53

make warning more specific

d9f1b0d

update test

fcb81fc

CagtayFabry commented Nov 5, 2021

View reviewed changes

CagtayFabry marked this pull request as ready for review November 5, 2021 12:39

Merge branch 'master' into check_time_coords

d2ee0f6

CagtayFabry requested a review from vhirtham November 5, 2021 12:40

vhirtham approved these changes Nov 5, 2021

View reviewed changes

CagtayFabry added 4 commits November 10, 2021 08:19

Merge branch 'master' into check_time_coords

4a21d79

transpose SpatialData on init, fix is_time_dependent

3e18f2e

update coordinate interpolation

2153d64

update CHANGELOG.rst

97677cf

CagtayFabry commented Nov 10, 2021

View reviewed changes

CagtayFabry requested a review from vhirtham November 10, 2021 08:21

vhirtham approved these changes Nov 10, 2021

View reviewed changes

CagtayFabry merged commit abe5cb3 into BAMWelDX:master Nov 10, 2021

CagtayFabry deleted the check_time_coords branch November 10, 2021 08:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check time coords #635

Check time coords #635

CagtayFabry commented Nov 4, 2021 •

edited

github-actions bot commented Nov 4, 2021 •

edited

codecov bot commented Nov 5, 2021 •

edited

CagtayFabry Nov 5, 2021

CagtayFabry Nov 5, 2021

CagtayFabry Nov 5, 2021

CagtayFabry Nov 5, 2021 •

edited

CagtayFabry Nov 10, 2021

vhirtham Nov 10, 2021 •

edited

CagtayFabry Nov 10, 2021 •

edited

vhirtham Nov 10, 2021

vhirtham Nov 10, 2021

CagtayFabry commented Nov 10, 2021

		assert lcs_interp.is_time_dependent is False
		assert lcs_interp.time.equals(Time("2s"))

		# make sure we have correct dimension order
		self.coordinates = self.coordinates.transpose(..., "n", "c")

Check time coords #635

Check time coords #635

Conversation

CagtayFabry commented Nov 4, 2021 • edited

Changes

Checks

github-actions bot commented Nov 4, 2021 • edited

Unit Test Results

codecov bot commented Nov 5, 2021 • edited

Codecov Report

CagtayFabry Nov 5, 2021

Choose a reason for hiding this comment

CagtayFabry Nov 5, 2021

Choose a reason for hiding this comment

CagtayFabry Nov 5, 2021

Choose a reason for hiding this comment

CagtayFabry Nov 5, 2021 • edited

Choose a reason for hiding this comment

CagtayFabry Nov 10, 2021

Choose a reason for hiding this comment

vhirtham Nov 10, 2021 • edited

Choose a reason for hiding this comment

CagtayFabry Nov 10, 2021 • edited

Choose a reason for hiding this comment

vhirtham Nov 10, 2021

Choose a reason for hiding this comment

vhirtham Nov 10, 2021

Choose a reason for hiding this comment

CagtayFabry commented Nov 10, 2021

CagtayFabry commented Nov 4, 2021 •

edited

github-actions bot commented Nov 4, 2021 •

edited

codecov bot commented Nov 5, 2021 •

edited

CagtayFabry Nov 5, 2021 •

edited

vhirtham Nov 10, 2021 •

edited

CagtayFabry Nov 10, 2021 •

edited