csm time interpolation range

[CagtayFabry]

Changes

Describe changes in this PR

Related Issues

Closes #289

Checks

• updated CHANGELOG.md
• updated tests
• updated doc/
• update example/tutorial notebooks

[CagtayFabry]

it probably makes sense to implement this in the get_cs_on_edge method

[codecov]

Codecov Report

Merging #476 (34a60f9) into master (c31df44) will increase coverage by 0.08%.
The diff coverage is 97.05%.

@@            Coverage Diff             @@
##           master     #476      +/-   ##
==========================================
+ Coverage   96.77%   96.86%   +0.08%     
==========================================
  Files          89       89              
  Lines        5519     5546      +27     
==========================================
+ Hits         5341     5372      +31     
+ Misses        178      174       -4     

Impacted Files	Coverage Δ
weldx/transformations/cs_manager.py	98.72% <ø> (ø)
weldx/transformations/local_cs.py	97.50% <97.05%> (-0.16%)	⬇️
weldx/util.py	98.36% <0.00%> (-0.28%)	⬇️
weldx/time.py	98.01% <0.00%> (+2.97%)	⬆️

---

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 c31df44...34a60f9. Read the comment docs.

[pep8speaks]

Hello @CagtayFabry! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-08-18 07:35:59 UTC

[vhirtham]

Need to do some cleanup and update the docstrings of the CSM and LCS methods to reflect the new behavior, but apart from that this should be ready. You can already have a look. I am not sure if there is a way to merge some of the conditions in LCS.interp_time but I think there isn't much room for optimizations because coordinates and orientations are pretty independent of each other. Maybe we can put their individual interpolations into small internal functions, but that would be just code cosmetics.

[CagtayFabry]

I took a quick look and I think the implementation looks good (and pretty readable now with the Time class), no need to micro manage there I guess 👍

however for some of these larger if branches and longer functions I sometimes find it easier to have a few inline comments about the code (just for readability), I'll add an example

[CagtayFabry]

eventually we could also implement stuff like 'partially' overlapping times, so LCS.time = [0,1,2,3,4,5] and interp_time=[3,4,5,6,7,8,9] would give back LCS.time=[3,4,5] (I hope that makes sens 😉 )

[vhirtham]

I streamlined the behavior of interp_time so that it will also return a static system if only a single time value is passed. Before we had a time-dependent system with a single value. Now the time and the extra array dimension are stripped in that case.

eventually we could also implement stuff like 'partially' overlapping times, so LCS.time = [0,1,2,3,4,5] and interp_time=[3,4,5,6,7,8,9] would give back LCS.time=[3,4,5] (I hope that makes sens 😉 )

I think this will make stuff too complicated. It might be easy to implement in the LCS, but it will be a nightmare in the CSM. We would have to check for this case separately and broadcast the missing values for the multiplication. But this would also require figuring out how many values were stripped on either side of the returned array. I would leave it as is.

[CagtayFabry]

I streamlined the behavior of interp_time so that it will also return a static system if only a single time value is passed. Before we had a time-dependent system with a single value. Now the time and the extra array dimension are stripped in that case.

Maybe we can keep the time as a dimension but not coordinate of the dataarray, I'll take a look

eventually we could also implement stuff like 'partially' overlapping times, so LCS.time = [0,1,2,3,4,5] and interp_time=[3,4,5,6,7,8,9] would give back LCS.time=[3,4,5] (I hope that makes sens 😉 )

I think this will make stuff too complicated. It might be easy to implement in the LCS, but it will be a nightmare in the CSM. We would have to check for this case separately and broadcast the missing values for the multiplication. But this would also require figuring out how many values were stripped on either side of the returned array. I would leave it as is.
agreed

[vhirtham]

Maybe we can keep the time as a dimension but not coordinate of the dataarray, I'll take a look

I don't think that this makes sense.

[CagtayFabry]

Maybe we can keep the time as a dimension but not coordinate of the dataarray, I'll take a look

I don't think that this makes sense.

Thought it could be nice to keep some time information but also not really needed right now I guess

[vhirtham]

@CagtayFabry Funny thing: I can't require your approval because you created the PR, but I can approve myself 😉 So are we good to go?

[CagtayFabry]

Maybe we can keep the time as a dimension but not coordinate of the dataarray, I'll take a look

I don't think that this makes sense.

Thought it could be nice to keep some time information but also not really needed right now I guess

looked into it a little and I think it would be good to differentiate between xarray coords and dimensions more in our codebase (currently we mostly use coords which is probably not what we actually want)

I'll see about it in a separate PR