-
-
Notifications
You must be signed in to change notification settings - Fork 72
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
✨ np.array interface implementation #154
Conversation
delta's: - used pytest parameterize - added lazy_fixture dependency to support lazy fixtures loading within the parametrization
Ready for review @jonasvdd? |
Now it is! @jvdd 🎉 |
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #154 +/- ##
==========================================
+ Coverage 97.00% 97.44% +0.43%
==========================================
Files 12 13 +1
Lines 902 978 +76
==========================================
+ Hits 875 953 +78
+ Misses 27 25 -2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a final review, will push a commit with some minor updates.
If we address the comments from this review, we can merge this one 🚀
Some general comments:
- Add tests for passing None when using range-index
- We should profile the code for unnecessary overhead
- Still not 100% sure whether we should have pandas as a core dependency 🤔
x_dtype_regex_list: Optional[List[str]] = None, | ||
y_dtype_regex_list: Optional[List[str]] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also present in tsdownsample? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will look into this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I woudl leave this as is for now; because:
- tsdownsample it's interface has a hard lock-in on returning the selected indices; which is only true for the
DataPointSelector
classes. - And since, we also support data aggregation e.g. mean / ...; serving a dtype regex to that function might be beneifical.
Main thing still left to do:
- look if there is any unnecessary overhead w.r.t. the dtype regex (e.g. we define them for both the
plotly-resampler
andtsdownsample
interface.) -> maybe we can just reusetsdownsample
itsinterface for the
datapoint-selector` class?
plotly_resampler/figure_resampler/figure_resampler_interface.py
Outdated
Show resolved
Hide resolved
plotly_resampler/figure_resampler/figure_resampler_interface.py
Outdated
Show resolved
Hide resolved
plotly_resampler/figure_resampler/figure_resampler_interface.py
Outdated
Show resolved
Hide resolved
refactor: use composition for gap handling
Ticked off all TODOs. @jonasvdd, could you please review this pull request one final time? If everything looks good, we can merge it into the main branch. After that, we can consider publishing a release candidate. 🚀 |
Changes applied:
pd.Series
dependency for theAggregators
and worked towards a numpy array Aggregation interface;As a result: The aggregation_interface.py class structure is now:
AbstractAggregator
superclass; implemented byDataPointSelector
: Selects indices (will be the superclass for tsdownsample aggregators)arg_downsample
methodDataAggregator
: Aggregates the x and yaggregate
method.MinaxLTTB
FigureResamplerInterface
its_check_update_trace_data
to a new (static) classPlotlyAggregatorParser