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.

Sign up for GitHub

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

Jump to bottom

Hvplot (timeseries) #198

Open
wants to merge 34 commits into
base: main
Choose a base branch
from
Open

Hvplot (timeseries) #198

wants to merge 34 commits into from

Conversation

sarahclaude
Copy link
Collaborator

@sarahclaude sarahclaude commented Apr 26, 2024 •
edited
Loading

Pull Request Checklist:

  • This PR addresses an already opened issue (for bug fixes / features)
    • This PR fixes #xyz
  • (If applicable) Documentation has been added / updated (for bug fixes / features).
  • (If applicable) Tests have been added.
  • CHANGES.rst has been updated (with summary of main changes).
    • Link to issue (:issue:number) and pull request (:pull:number) has been added.

What kind of change does this PR introduce?

Adding a wraper around hvplot in figanos

  • Ouranos bokeh theme yaml
  • Timeseries function similar to figanos.matplotlib.timeseries()
  • Utilitaries functions used in timeseries
  • figanos_hvplot.ipynb presents exemples

Does this PR introduce a breaking change?

No

Other information:

Improvements:

  • show_lat_lon; screen units are not a good match with the width/height of plot needs to be improved
  • when multiple lines on the same points shows multiple mouse hover boxes
  • speed performance was not tested in a dashboard / panel app

To be added/done:

  • how to integrate hvplot with matplotlib (example: with a set_options function?) in figanos
  • keeps the docs splitted or make one doc with matplotlib/hvplot?
  • add traduction

@sarahclaude
bokeh_theme yaml similar to ouranos matplotlib stylesheet
4c78ddf
@sarahclaude
update bijeh, env, __init__
c4f5245
@sarahclaude
Merge branch 'main' into hvplot
9730f94
@sarahclaude
timseries fucntion in hvplot
29eaa7e
@sarahclaude
Merge branch 'main' into hvplot
bddedb4
@sarahclaude
Added set style function, added hvplot to __init__
f189f5a
@sarahclaude
Merge branch 'main' into hvplot
2e6c0af
@sarahclaude
fix bugs
0fbcebc
@sarahclaude
fix bug ens_perc
1a0291c
@sarahclaude
fix bug ens_stat
0c837ed
@sarahclaude
fix formatters and hover
ebe36a9
@sarahclaude
fix cplot_kw and hooks for hover area
bba460c
@sarahclaude
fix bug
8cb116b
@sarahclaude
merge main and moved hvplot to src/figanos
bf038e0
@sarahclaude
fix curve_hover_hook (loop with hover_tools instead of hover)
c449628
@sarahclaude
fix curve_hover_hook and added curve_hover_tool as other option
9f26871
@sarahclaude
added show_lat_lon to timeseries
59ac7a1
@sarahclaude
Merge branch 'main' into hvplot
8be59e7
@sarahclaude
added legend inplot to timeseries and notebook examples
39896a8
@review-notebook-app Review Notebook App
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@github-actions github-actions bot added the docs label Sep 17, 2024
@sarahclaude sarahclaude marked this pull request as ready for review October 1, 2024 21:01
juliettelavoie
juliettelavoie reviewed Oct 2, 2024
View reviewed changes
src/figanos/hvplot/style/ouranos.yml

Title:
text_color: black
text_font: DejaVu Sans
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the title doesn't look like DejaVu Sans... It looks like only the legend use the right font, the rest looks like times new roman or another serif font.

src/figanos/hvplot/plot.py Show resolved Hide resolved
src/figanos/hvplot/plot.py Show resolved Hide resolved
src/figanos/hvplot/plot.py
Arguments to pass to the `holoviews/hvplot.opts()` function. Changes figure options and access to hooks.
If 'data' is a dictionary, must be a nested dictionary with the same keys as 'data' to pass nested to each
individual figure or key 'overlay' to pass to overlayed figures.
legend : str (default 'lines') or dict
Copy link
Contributor

Choose a reason for hiding this comment

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

legend='none' is not working

juliettelavoie
juliettelavoie reviewed Oct 2, 2024
View reviewed changes
docs/notebooks/figanos_hvplot.ipynb
@@ -0,0 +1,194 @@
{
Copy link
Contributor

@juliettelavoie juliettelavoie Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Line #10.                                          #'group',

clean up the commented line


Reply via ReviewNB

docs/notebooks/figanos_hvplot.ipynb
@@ -0,0 +1,194 @@
{
Copy link
Contributor

@juliettelavoie juliettelavoie Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Line #1.    from figanos.hvplot import timeseries

suggestion: au top mettre import figanos.hvplot as fgh et import figanos.matplotlig as fgm et ensuite faire fgh.timeseries et fgm.timeseries pour montrer que le call est presque pareil.


Reply via ReviewNB

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Bonne idée! Seulement dans la doc hvplot ou toutes les docs?

Copy link
Contributor

Choose a reason for hiding this comment

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

je dirais juste hvplot

docs/notebooks/figanos_hvplot.ipynb
@@ -0,0 +1,194 @@
{
Copy link
Contributor

@juliettelavoie juliettelavoie Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Line #1.    timeseries(data2, legend='in_plot', show_lat_lon='lower left')

pourquoi quand c'est in_plot la legend est "realisation:0", mais lines est seulement "0"


Reply via ReviewNB

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

lines est fait automatiquement par hvplot; je n'ai pas modifier comment il créait les légendes. Alors, que in_plot j'ai ajouté la fonction et j'ai essayé de mettre les mêmes options que pour matplotlib, mais peut-être que c'est incohérent.

Copy link
Contributor

Choose a reason for hiding this comment

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

Je pense que ce serait plus beau avec juste "0", sans le nom de la dimension. Il me semble qu'on ne voit jamais le nom de la dim dans matplolib ?

docs/notebooks/figanos_hvplot.ipynb
@@ -0,0 +1,194 @@
{
Copy link
Contributor

@juliettelavoie juliettelavoie Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Je mettrais chaque arg sur une ligne différente pour que ça soit plus facile à lire.


Reply via ReviewNB

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juliettelavoie juliettelavoie juliettelavoie left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sarahclaude @juliettelavoie @Zeitsperre