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

📦 improve docs #104

Merged
merged 50 commits into from
Aug 11, 2022
Merged

📦 improve docs #104

merged 50 commits into from
Aug 11, 2022

Conversation

jvdd
Copy link
Member

@jvdd jvdd commented Jul 17, 2022

This PR aims at improving the documentation of this package, as we have had several issues about the lacking documentation (#99, #91, #102)

  • create FAQ
  • update Readme.md
  • add Contributing.md
  • add Changelog.md
  • add requirements.txt to example folder
  • improve dash integration docs
  • add very minimal dash integration example
  • update example notebooks
  • add more documentation to the C code
  • incorporate figure serialization into the docs

Other stuff this PR does;

incorporate Plotly-resampler's LTTBc bindings

TODO:

  • test lttb_core_py
  • test the EfficientLTTB method when LTTB_core_c is not available
    note: this method is tested for equality with the lttbc method

@codecov-commenter
Copy link

codecov-commenter commented Jul 17, 2022

Codecov Report

Merging #104 (070e6fc) into main (5b7bcd0) will decrease coverage by 0.70%.
The diff coverage is 83.67%.

@@            Coverage Diff             @@
##             main     #104      +/-   ##
==========================================
- Coverage   97.63%   96.93%   -0.71%     
==========================================
  Files          10       12       +2     
  Lines         804      848      +44     
==========================================
+ Hits          785      822      +37     
- Misses         19       26       +7     
Impacted Files Coverage Δ
plotly_resampler/figure_resampler/utils.py 96.29% <50.00%> (-3.71%) ⬇️
plotly_resampler/aggregation/aggregators.py 91.66% <53.84%> (-6.75%) ⬇️
plotly_resampler/aggregation/algorithms/lttb_c.py 100.00% <100.00%> (ø)
plotly_resampler/aggregation/algorithms/lttb_py.py 100.00% <100.00%> (ø)
...ler/figure_resampler/figure_resampler_interface.py 99.71% <0.00%> (ø)

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

@jonasvdd jonasvdd added documentation Improvements or additions to documentation enhancement New feature or request labels Jul 18, 2022
Co-authored-by: jvdd [email protected]
@jonasvdd
Copy link
Member

Ready for review @jvdd!

@SirVer
Copy link

SirVer commented Aug 4, 2022

I looked through the examples in this MR and ran them on my system and I think this is a huge improvement:

  • 01_minimal_global.py worked out of the box for me, has minimal dependencies and a lot of really useful comments explaining what is going on, why having a global is bad practice. It contains the gist of how the library is meant to be used. Really great!
  • 02_minimal_cache.py seems to build logically on 01, infact looking at the diff between the files nicely shows the differences needed for caching. however, when running it and zooming a bit I get an error (traceback in this Gist).
  • 03_minimal_cache_dynamic.py is clear in its instructions, but not in its motivation. Why would I want to use a dcc.Store in the first place? When is this useful? Running it I get a different error when zooming the dynamically added graph, but similar looking error to above (ValueError: Invalid property specified for object of type plotly.graph_objs.Scatter: 'data').

I did not look at the remaining examples, they seemed much more advanced to me.

Great job at improving the examples! I'll go ahead and try to implement this in my app now again.

@jonasvdd
Copy link
Member

jonasvdd commented Aug 4, 2022

Hi @SirVer,

I cannot seem to reproduce your experienced issues; could you share a pip freeze; so my python environment is an exact copy of yours?

@SirVer
Copy link

SirVer commented Aug 4, 2022

@jonasvdd Here is a gist with the Pipfile & Pipfile.lock that made my environment.

I did the following:

  1. cd to /tmp/plotly-resampler/examples/dash_apps, which had this branch checkout out.
  2. pipenv shell
  3. pipenv install dash plotly_resampler
  4. python 02_minimal_cache.py

@jonasvdd
Copy link
Member

jonasvdd commented Aug 4, 2022

Thanks @SirVer! I was able to reproduce the error. 🥲

I think that the serialization support was not yet integrated into 0.7.2.2

If I execute pipenv install plotly-resampler==0.8.0rc12 and then run the examples; they work flawlessly!

@jonasvdd
Copy link
Member

jonasvdd commented Aug 7, 2022

@jvdd; ready for review!

@jonasvdd jonasvdd self-requested a review August 7, 2022 08:16
Copy link
Member Author

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

LGTM! Very nice work, quality of docs is improved significantly 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants