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

✨ np.array interface implementation #154

Merged
merged 54 commits into from
Apr 24, 2023
Merged

✨ np.array interface implementation #154

merged 54 commits into from
Apr 24, 2023

Conversation

jonasvdd
Copy link
Member

@jonasvdd jonasvdd commented Dec 31, 2022

Changes applied:

  • removed the pd.Series dependency for the Aggregators and worked towards a numpy array Aggregation interface;

As a result: The aggregation_interface.py class structure is now:

  • AbstractAggregator superclass; implemented by
    • DataPointSelector : Selects indices (will be the superclass for tsdownsample aggregators)
      • contains the arg_downsample method
    • DataAggregator: Aggregates the x and y
      • contains the aggregate method.

image

@jonasvdd jonasvdd requested a review from jvdd December 31, 2022 16:52
@jonasvdd jonasvdd changed the title ✨: np.array interface implementation ✨ np.array interface implementation Jan 3, 2023
@jvdd
Copy link
Member

jvdd commented Jan 31, 2023

Ready for review @jonasvdd?

@jonasvdd jonasvdd marked this pull request as ready for review January 31, 2023 19:48
@jonasvdd
Copy link
Member Author

Now it is! @jvdd 🎉

This was referenced Feb 2, 2023
@codecov-commenter
Copy link

codecov-commenter commented Feb 4, 2023

Codecov Report

Merging #154 (497b2a2) into main (b1225bd) will increase coverage by 0.43%.
The diff coverage is 98.15%.

📣 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     
Impacted Files Coverage Δ
...tly_resampler/aggregation/gap_handler_interface.py 95.00% <95.00%> (ø)
...tly_resampler/aggregation/aggregation_interface.py 95.00% <96.00%> (-3.47%) ⬇️
..._resampler/aggregation/plotly_aggregator_parser.py 96.62% <96.62%> (ø)
plotly_resampler/__init__.py 84.61% <100.00%> (ø)
plotly_resampler/aggregation/__init__.py 100.00% <100.00%> (ø)
plotly_resampler/aggregation/aggregators.py 100.00% <100.00%> (+8.33%) ⬆️
plotly_resampler/aggregation/gap_handlers.py 100.00% <100.00%> (ø)
...tly_resampler/figure_resampler/figure_resampler.py 89.16% <100.00%> (ø)
...ler/figure_resampler/figure_resampler_interface.py 100.00% <100.00%> (+0.26%) ⬆️
...sampler/figure_resampler/figurewidget_resampler.py 99.03% <100.00%> (ø)
... and 1 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@jvdd jvdd left a 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 🤔

pyproject.toml Outdated Show resolved Hide resolved
tests/utils.py Show resolved Hide resolved
docs/sphinx/FAQ.rst Show resolved Hide resolved
Comment on lines +18 to +19
x_dtype_regex_list: Optional[List[str]] = None,
y_dtype_regex_list: Optional[List[str]] = None,
Copy link
Member

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? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

will look into this!

Copy link
Member Author

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:

  1. tsdownsample it's interface has a hard lock-in on returning the selected indices; which is only true for the DataPointSelector classes.
  2. 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 and tsdownsample interface.) -> maybe we can just reuse tsdownsample its interface for the datapoint-selector` class?

plotly_resampler/aggregation/aggregators.py Outdated Show resolved Hide resolved
@jvdd jvdd mentioned this pull request Apr 20, 2023
@jvdd
Copy link
Member

jvdd commented Apr 24, 2023

Ticked off all TODOs.
I'm fine with atm not having versioned docs - is mostly relevant for LTTB.c code (which is now removed and replaced by tsdownsample)

@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. 🚀

@jonasvdd jonasvdd merged commit 03420ed into main Apr 24, 2023
16 checks passed
@jvdd jvdd deleted the np_array_interface branch June 18, 2023 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Optimization Aims to make the toolkit more memory / computationally efficient
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Could you pls relax pandas requirement pandas = "^1.3.5"? pandas==2.0.0 was released!
4 participants