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

Update TimeSeries handling of absolute times #791

Merged
merged 14 commits into from
Jul 27, 2022

Conversation

vhirtham
Copy link
Collaborator

@vhirtham vhirtham commented Jul 25, 2022

Changes

Addresses the issues mentioned in #471

Related Issues

Closes #471

Checks

  • updated CHANGELOG.rst
  • updated tests
  • updated doc/
  • update example/tutorial notebooks
  • update manifest file

@vhirtham vhirtham added core weldx core classes and functions refactor labels Jul 25, 2022
@github-actions
Copy link

github-actions bot commented Jul 25, 2022

Test Results

2 185 tests  +31   2 184 ✔️ +30   3m 34s ⏱️ -26s
       1 suites ±  0          1 💤 +  1 
       1 files   ±  0          0 ±  0 

Results for commit 3b03955. ± Comparison against base commit 2245e0e.

♻️ This comment has been updated with latest results.

@codecov
Copy link

codecov bot commented Jul 25, 2022

Codecov Report

Merging #791 (3b03955) into master (2245e0e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #791   +/-   ##
=======================================
  Coverage   96.87%   96.87%           
=======================================
  Files          88       88           
  Lines        6019     6025    +6     
=======================================
+ Hits         5831     5837    +6     
  Misses        188      188           
Impacted Files Coverage Δ
weldx/core.py 93.45% <100.00%> (+0.04%) ⬆️
weldx/time.py 97.78% <100.00%> (+0.01%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us.

weldx/core.py Outdated
Comment on lines 488 to 490
if self.time is None and time.is_absolute:
# type: ignore[union-attr]
data = data.weldx.reset_reference_time(time.reference_time)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something we should probably discuss. It fixes the following problem:

We replaced

        return ut.xr_interp_like(
            self._data,
            {"time": time.as_data_array()},
            method=self.interpolation,
            assume_sorted=False,
            broadcast_missing=False,
        )

with

return ut.xr_interp_like(
            data,
            time.as_data_array(),
            method=self.interpolation,
            assume_sorted=False,
            broadcast_missing=False,
        )

to get absolute times working correctly when using xr_interp_like. However, this causes the test case (ts_const, time_mul + pd.Timestamp("2020"), [1, 1, 1, 1, 1, 1, 1, 1], "m") of test_interp_time to fail (currently line 450 in test_core.py). The test just checks if the value of a constant TimeSeries that gets interpolated with an array of times is broadcasted correctly. Since the value is constant, reference times shouldn't matter, but after the change, they suddenly did. So I caught this special case and just added a reference time to a copy of the data prior to interpolation. The question is if we want to keep it that way, should it be handled differently or is that something that xr_interp_like should handle internally

@vhirtham vhirtham marked this pull request as ready for review July 26, 2022 13:59
@vhirtham vhirtham merged commit bbab081 into BAMWelDX:master Jul 27, 2022
@vhirtham vhirtham deleted the 471_timeseries branch July 27, 2022 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core weldx core classes and functions refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TimeSeries todos
1 participant