Timeseries plot style #465

FrejaTerpPetersen · 2024-11-06T12:40:18Z

An update to the comparer plotter such that it takes a style and a color input. I wanted to also allow for color input, since this is actually the main reason why I started the issue #463: I want to use my own colors to match my other plots. The style input is just a bonus from my point of view :)

Design decisions:

The user can only pass one of the two; either color OR style
If none are passed, the default colors are used.
Inputs can be passed as list of strings or as a single string. If a single string is passed but more than one model result is in the comparer, an error is raised. If len(list) < num_models an error is also raised.
If len(color) > num_models: the first color is used for the observations. I am unsure whether this is the best way to do it, since there might be cases, where I just have a large list of colors that I pass to all of my plots regardless of the number of models... On the other hand, allowing the user to choose the observation colors is also useful in case of duplicate colors (default obs color is black, and maybe I want one of the lines to be black too?)

Example 1: giving a color input

# Given a comparer:
cmp = create_comparer()
# Pass a color input to the plot.timeseries() method
ax = cmp.plot.timeseries(color=["red", "blue"])
plt.show()

This code yields the following plot:

Example 2: also specifying the color of the observation points
Above, the default color of the observations is black. If we want them to be e.g. green instead, we pass it as the first color.

# Pass a color input to the plot.timeseries() method
ax = cmp.plot.timeseries(color=["green","red", "blue"])
plt.show()

This yields the following plot:

Example 3: giving a style input
We can also pass a style input instead

# Pass a color input to the plot.timeseries() method
ax = cmp.plot.timeseries(style=["r-.", "b--"])
plt.show()

ecomodeller · 2024-11-06T15:43:44Z

@FrejaTerpPetersen for the benefit of the interested reader... could you please include some sample code and screenshots of the new functionality in this description above. Visuals are easy to grasp, when browsing around.

FrejaTerpPetersen · 2024-11-11T09:03:21Z

Sure thing - I updated the description

ecomodeller · 2024-11-13T08:45:16Z

modelskill/comparison/_comparer_plotter.py

+                else:
+                    if color is None:
+                        color = MOD_COLORS
+                    mod.plot(ax=ax, color=color[j])

            ax.scatter(
                cmp.time,
                cmp.data[cmp._obs_name].values,
                marker=".",


style does not affect the observations, is this the intended behaviour?

Yes, observations can't have their style changed now. THis is a design choice, I guess? We could allow the user to input style for obs? Might lead to confusion, however, since I guess the user is pretty happy with a dot for the obs

I see the color can be changed. But not the marker.

modelskill/comparison/_comparer_plotter.py

jsmariegaard · 2024-11-20T07:11:41Z

modelskill/comparison/_comparer_plotter.py

+        # Color for observations:
+        obs_color = cmp.data[cmp._obs_name].attrs["color"]
+
+        # if color is None and style is None:  # Use default values for colors


remove unused code

jsmariegaard

Great work!

I would like to see a few changes:

please change _check_kwarg_and_convert_to_list to only check for either style or color and rename to _check_arg_length_match_nmodels(arg, arg_name, n_mod) or to something with _parse (as it returns the same argument again)
move the function out of _misc.py and to top of _comparer_plotter.py (it is only used in this module)
I think we should allow either lengths n_mod and n_mod+1 only (i.e. also issue exception if length is too long)
It seems that the color and style args work a little different if you want to change the obs apperance also. Is it correct that you cannot change the style of the observation.
I guess the style and color arguments only applied to matplotlib backend and not plotly. If that is the case - it should be clear in the docstring. Better: add it to plotly backend also.
currently all kwargs are accepted, e.g. cmp.plot.timeseries(assdsddsdas="Reds") does not give me an error message. It should. It is not strictly part of this PR, but as this function is now "open", we should fix it.
I have added a short notebook with examples - maybe you could expand it a little bit?

jsmariegaard · 2024-12-18T10:08:31Z

closes #463

ecomodeller · 2025-03-11T10:25:31Z

@FrejaTerpPetersen What is the status on this PR?

FrejaTerpPetersen · 2025-03-12T07:52:53Z

@ecomodeller I think it is ready for merge, unless you have further comments.

I updated according to Jespers comments:

please change _check_kwarg_and_convert_to_list to only check for either style or color and rename to _check_arg_length_match_nmodels(arg, arg_name, n_mod) or to something with _parse (as it returns the same argument again)
->I changed the function to your first suggestion. Not familiar with '_parse' - what would that mean?
move the function out of _misc.py and to top of _comparer_plotter.py (it is only used in this module)
->Done
I think we should allow either lengths n_mod and n_mod+1 only (i.e. also issue exception if length is too long)
-> Hmmm, my reason for allowing more colors is that the user of the function might just have some generic list of colors from a theme that they want to input without subsetting. But maybe that is an unusual use-case?
I guess the style and color arguments only applied to matplotlib backend and not plotly. If that is the case - it should be clear in the docstring. Better: add it to plotly backend also.
-> I changed docstring - not familiar with plotly so can't quickly do this.
currently all kwargs are accepted, e.g. cmp.plot.timeseries(assdsddsdas="Reds") does not give me an error message. It should. It is not strictly part of this PR, but as this function is now "open", we should fix it.
-> Hmm, not sure I understand exactly ...

@ecomodeller, @jsmariegaard - do you want to review before merging?

jsmariegaard · 2025-03-12T08:40:15Z

Yes, either @ecomodeller or I will merge after review. Thanks

ecomodeller · 2025-03-12T19:57:20Z

modelskill/comparison/_comparer_plotter.py

+    if isinstance(arg, str):
+        # If arg is str, convert to list (for looping)
+        arg = [arg]
+    if arg is not None and len(arg) < n_mod:


This doesn't account for the case where the user passes more arguments than n_mod. This should not be silently ignored.

FrejaTerpPetersen added 9 commits November 5, 2024 14:06

Style argument to timeseries plot

27ac391

first value is obs style

c972683

Added color argument

3619190

Created function for checking inputs

72d3210

Decided to only allow color OR style input

dfef139

Wrote tests

a68fcfd

Deleted test call

320563d

docstring update

d55b4c7

Design decision

d2d57e3

FrejaTerpPetersen requested review from ecomodeller and jsmariegaard November 6, 2024 12:40

FrejaTerpPetersen added 6 commits November 6, 2024 13:41

Removed unused import

4446709

Fix type error: color was not subscriptable if None

3a1fcba

Wrong type error ... Moved default coloring

0f57302

Import default colors

1a70efb

Allow color to be tuple

13a7163

Moved import outside plot code

3f941aa

ecomodeller requested changes Nov 13, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into timeseries-plot-style

f4d22ff

jsmariegaard reviewed Nov 20, 2024

View reviewed changes

Example notebook with timeseries plotting

5cf2d03

jsmariegaard requested changes Nov 20, 2024

View reviewed changes

jsmariegaard linked an issue Dec 18, 2024 that may be closed by this pull request

Passing matplotlib arguments to plot.timeseries() #463

Open

FrejaTerpPetersen added 3 commits March 12, 2025 08:33

Specify only matplotlib backend in docstring

9337b8e

Moved check function from misc to top of comparer_plotter

fb44f8a

Rewrite check function

6b4476c

remove unused code, update test

5c0a029

ecomodeller added 3 commits March 12, 2025 18:55

Not (yet) implemented in plotly

9b69219

Add plotly as test dependency

33f631a

Merge branch 'main' into timeseries-plot-style

dc858ea

ecomodeller reviewed Mar 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Timeseries plot style #465

Timeseries plot style #465

FrejaTerpPetersen commented Nov 6, 2024 •

edited by ecomodeller

Loading

ecomodeller commented Nov 6, 2024

FrejaTerpPetersen commented Nov 11, 2024

ecomodeller Nov 13, 2024

FrejaTerpPetersen Mar 12, 2025 •

edited

Loading

ecomodeller Mar 12, 2025

jsmariegaard Nov 20, 2024

jsmariegaard left a comment

jsmariegaard commented Dec 18, 2024

ecomodeller commented Mar 11, 2025

FrejaTerpPetersen commented Mar 12, 2025

jsmariegaard commented Mar 12, 2025

ecomodeller Mar 12, 2025

Timeseries plot style #465

Are you sure you want to change the base?

Timeseries plot style #465

Conversation

FrejaTerpPetersen commented Nov 6, 2024 • edited by ecomodeller Loading

ecomodeller commented Nov 6, 2024

FrejaTerpPetersen commented Nov 11, 2024

ecomodeller Nov 13, 2024

Choose a reason for hiding this comment

FrejaTerpPetersen Mar 12, 2025 • edited Loading

Choose a reason for hiding this comment

ecomodeller Mar 12, 2025

Choose a reason for hiding this comment

jsmariegaard Nov 20, 2024

Choose a reason for hiding this comment

jsmariegaard left a comment

Choose a reason for hiding this comment

jsmariegaard commented Dec 18, 2024

ecomodeller commented Mar 11, 2025

FrejaTerpPetersen commented Mar 12, 2025

jsmariegaard commented Mar 12, 2025

ecomodeller Mar 12, 2025

Choose a reason for hiding this comment

FrejaTerpPetersen commented Nov 6, 2024 •

edited by ecomodeller

Loading

FrejaTerpPetersen Mar 12, 2025 •

edited

Loading