Tick and tick calendar include left and right #376
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
javiber
force-pushed
the
optimize-calendar-ops
branch
2 times, most recently
from
6c72287
to
1e937ab
Compare
javiber
force-pushed
the
calendar-tick-fixes
branch
2 times, most recently
from
ba27208
to
e16466c
Compare
achoum
reviewed
Feb 27, 2024
| @@ -41,14 +41,15 @@ def __call__(self, input: EventSet) -> Dict[str, EventSet]: | |||
|
|
|||
| # fill output EventSet data | |||
| for index_key, index_data in input.data.items(): | |||
| if len(index_data.timestamps) == 0: | |||
| if len(index_data.timestamps) <= 1: | |||
There was a problem hiding this comment.
For brainstorming: Should there be cases where at least one event is returned?
There was a problem hiding this comment.
Changed this, the behavior now is similar to what we have currently on main.
Check the new test: https://github.com/google/temporian/pull/376/files#diff-22438ae7b5327b043eb72f32e896dc62eb5c44d523d3535a0f3b65c1339894a1R57
javiber
force-pushed
the
optimize-calendar-ops
branch
5 times, most recently
from
ab36c41
to
9dae7ad
Compare
javiber
force-pushed
the
calendar-tick-fixes
branch
2 times, most recently
from
1e2eb6e
to
d06edf4
Compare
javiber
force-pushed
the
calendar-tick-fixes
branch
3 times, most recently
from
36818a8
to
8937bd7
Compare
Coverage reportMain: 91.32% | PR: 91.35% | Diff: 0.04 ✅ |
2 similar comments
Coverage reportMain: 91.32% | PR: 91.35% | Diff: 0.04 ✅ |
Coverage reportMain: 91.32% | PR: 91.35% | Diff: 0.04 ✅ |
javiber
force-pushed
the
calendar-tick-fixes
branch
from
8937bd7
to
a797c66
Compare
Coverage reportMain: 91.32% | PR: 91.35% | Diff: 0.04 ✅ |
ianspektor
approved these changes
Mar 20, 2024
Coverage reportMain: 91.57% | PR: 91.35% | Diff: -0.21 |
Had to change the end in tick because nextafter was not working for small values (in the -0.1-0 range)
- added examples using these params to docs - updated old examples - changed the behavior of tick when a single event is passed, the current behavior is more similar to the one before the new parameters - fixed a bug on tick_calendar with a single element - added tests for both operators testing the behavior for single-event inputs under different parameters - added test for empty eventset on tick_calendar - added changelog
javiber
force-pushed
the
calendar-tick-fixes
branch
from
c3df964
to
c6d70bc
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Trueby defaultFalseby defaultChanged the behavior of the tick operator when input has 1 item, now it always returns an empty event set (used to return one element ifalign=Falseand empty otherwise)Calendar ops needs to be merged first