Refactor interpolation #486
Conversation
# Conflicts: # .github/workflows/pytest.yml # .github/workflows/pytest_asdf.yml # .github/workflows/static_analysis.yml
| else: # full interpolation with overlapping times | ||
| coordinates = ut.xr_interp_coordinates_in_time(self.coordinates, time) | ||
|
|
||
| if len(coordinates) == 1: # remove "time dimension" for single value cases |
There was a problem hiding this comment.
The len(coordinates) == 1 check can be pretty unstable imo @vhirtham
at first I was surprised this works without even knowing what it does. Using len(coordinates.time) == 1 here is probably what we want.
The problem is we cannot (or don't want to) guarantee that time is actually the first dimension
There was a problem hiding this comment.
Well, this is why you find this specific line below the processing of the inputs. coordinates and orientations should already be formatted correctly to match the specific internal structure I was abusing here. So it is pretty stable I guess unless you change the order of instructions in the __init__. But in this case, we have a ton of tests that will fail.
Not sure about len(coordinates.time) == 1. You might need to check the existence of coordinates.time first since static coordinates shouldn't have this attribute. Checking the passed time classes length shouldn't help much either since it gives no indication if coordinates actually have this extra dimension. I guess we can get it more self-explaining but at the price of additional checks. Not sure if it is worth it for class internal code that already, has a comment to state what it does.
There was a problem hiding this comment.
Well, this is why you find this specific line below the processing of the inputs.
coordinatesandorientationsshould already be formatted correctly to match the specific internal structure I was abusing here. So it is pretty stable I guess unless you change the order of instructions in the__init__. But in this case, we have a ton of tests that will fail.
I don't think the current __init__ prevents passing additional dimensions in coordinate dimensions for example?
Not sure about
len(coordinates.time) == 1. You might need to check the existence ofcoordinates.timefirst since staticcoordinatesshouldn't have this attribute. Checking the passedtimeclasses length shouldn't help much either since it gives no indication ifcoordinatesactually have this extra dimension. I guess we can get it more self-explaining but at the price of additional checks. Not sure if it is worth it for class internal code that already, has a comment to state what it does.
wouldn't len(coordinates) == 1 also fail here if there was no time dimension?
len(LocalCoordinateSystem().coordinates) == 3
There was a problem hiding this comment.
I don't think the current
__init__prevents passing additional dimensions in coordinate dimensions for example?Just checked
_build_coordinatesand it seems that you are correct. Currently, aDataArraywill be just passed through if I didn't miss something. But I consider this as a bug that should be fixed. The LCS shouldn't store any data that is not necessary to do its job
I disagree, the LCS should work perfectly fine for all data arrays that meet the required layout. Why would we restrict it any further?
If memory is a concern I will gladly leave that to the user (and it should be an easy fix) since our internal methods don't add any unnecessary overhead. We don't have to guarantee restoring all and everything on serialization etc. but that is not the point if you simply want to 'use' the LCS class for whatever purpose
weldx/util.py
Outdated
|
|
||
| return dsx_out.transpose(..., "c", "v") | ||
| if len(da.time) == 1: # remove "time dimension" for static case | ||
| return da.isel({"time": 0}) |
There was a problem hiding this comment.
This is the replacement we should use instead of return coordinates[0].values or even return coordinates[0], explicitly naming the dimension
There was a problem hiding this comment.
I agreed with you in the case of this generalized function. However, as the internal structure in the LCS was well defined, accessing per index was safe and probably faster (no lookups).
Codecov Report
@@ Coverage Diff @@
## master #486 +/- ##
==========================================
- Coverage 96.84% 96.80% -0.04%
==========================================
Files 89 91 +2
Lines 5548 5548
==========================================
- Hits 5373 5371 -2
- Misses 175 177 +2
Continue to review full report at Codecov.
|
The thing is that without using explicit as for performance, I would really like to see a benchmark first for these tiny changes to consider this 😉 |
I am just talking about the LCS internals. I don't think it will ever need more than those 3 dimensions. But nevermind. As I said, we can do it.
No need for a benchmark. Direct access is always the fastest data access method you can have, but it won't matter much since its contribution to the total execution time of our scripts, it is probably irrelevant. 😉 |
# Conflicts: # CHANGELOG.md # weldx/tests/test_transformations.py # weldx/util/xarray.py
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
Hello @CagtayFabry! Thanks for updating this PR.
|
Changes
I have reworked the xarray handling a little to now work with xarray
dimensionsinstead ofcoordinateswhich is probably what we actually want. Mentioned here #476 (comment)It is now possible to keep e.g. a single timestamp as an xarray coordinate in a constant
orientationswithshape=[3,3]Related Issues
Closes # (add issue numbers)
Checks