Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

modify CSM merge for non datetime subsytems #268

Merged
merged 8 commits into from
Mar 11, 2021

Conversation

CagtayFabry
Copy link
Member

@CagtayFabry CagtayFabry commented Mar 11, 2021

Changes

  • allow time dependent subsystems that don't use absolute times to be merged into CSM instances with a set reference time
  • make sure csm.time_union() returns correct index type without explicit reference time

Related Issues

Closes # (add issue numbers)

Checks

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

@codecov
Copy link

codecov bot commented Mar 11, 2021

Codecov Report

Merging #268 (045047b) into master (ac9573d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #268   +/-   ##
=======================================
  Coverage   99.65%   99.65%           
=======================================
  Files          75       75           
  Lines        4019     4022    +3     
=======================================
+ Hits         4005     4008    +3     
  Misses         14       14           
Impacted Files Coverage Δ
weldx/transformations/cs_manager.py 98.61% <100.00%> (+<0.01%) ⬆️

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 ac9573d...045047b. Read the comment docs.

@CagtayFabry
Copy link
Member Author

CagtayFabry commented Mar 11, 2021

@vhirtham the problem is caused when csm.uses_absolute_times is true (there is some DateTimeIndex involved) but csm.reference_time is false (there is no reference time set on a CSM level)

I think this can only happen with a single reference_time LCS involved but not sure about the checks

This fix will return a valid pd.DateTimeIndex in these cases

@CagtayFabry CagtayFabry changed the title determine reference time for single LCS edge case modify CSM merge for non datetime subsytems Mar 11, 2021
@CagtayFabry
Copy link
Member Author

@vhirtham changed error behavior of csm.merge() as discussed

@CagtayFabry CagtayFabry added bug Something isn't working transformations everything related to the LCS / CSM labels Mar 11, 2021
@CagtayFabry CagtayFabry marked this pull request as ready for review March 11, 2021 16:06
@CagtayFabry CagtayFabry self-assigned this Mar 11, 2021
@CagtayFabry CagtayFabry merged commit 9070149 into BAMWelDX:master Mar 11, 2021
@CagtayFabry CagtayFabry deleted the fix_csm.time_union branch March 11, 2021 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working transformations everything related to the LCS / CSM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants