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

Sequential plot display with unified postage stamp plots #379

Merged
merged 18 commits into from
Mar 25, 2024

Conversation

jfrost-mo
Copy link
Member

@jfrost-mo jfrost-mo commented Jan 29, 2024

Example of output: https://tmpweb.net/k7pKV1n2XtNq/ (Valid until 2024-02-23)

Fixes #307.

@jfrost-mo jfrost-mo self-assigned this Jan 29, 2024
@jfrost-mo jfrost-mo added the enhancement New feature or request label Jan 29, 2024
Copy link
Contributor

github-actions bot commented Jan 29, 2024

Coverage

@jfrost-mo jfrost-mo force-pushed the 307_sequential_plot_display branch 2 times, most recently from e08729e to 8b9a1f1 Compare January 29, 2024 11:39
@jfrost-mo jfrost-mo changed the base branch from main to 296_recipe_parameterisation January 29, 2024 12:03
@jfrost-mo jfrost-mo changed the base branch from 296_recipe_parameterisation to main January 29, 2024 13:43
@jfrost-mo

This comment was marked as outdated.

@jfrost-mo jfrost-mo marked this pull request as ready for review January 30, 2024 09:24
@jfrost-mo
Copy link
Member Author

jfrost-mo commented Jan 30, 2024

Do we want the postage stamp plotting code merged into the same operator, so there is just a single spacial plot operator?

I think we want to do that, but in a second PR, as this one is already quite big. Issue #381 exists for this.

EDIT: I did it anyway, and it made more sense in the one PR.

src/CSET/_common.py Outdated Show resolved Hide resolved
src/CSET/operators/plot.py Outdated Show resolved Hide resolved
src/CSET/operators/plot.py Show resolved Hide resolved
@jfrost-mo jfrost-mo changed the title Sequential plot display Sequential plot display with uunified postage stamp plots Feb 16, 2024
@jfrost-mo jfrost-mo changed the title Sequential plot display with uunified postage stamp plots Sequential plot display with unified postage stamp plots Feb 16, 2024
Copy link
Collaborator

@JorgeBornemann JorgeBornemann left a comment

Choose a reason for hiding this comment

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

Changes look good, though note that I have not been able to test them here. However, I am holding approval pending a discussion on creation additional external dependencies (in this case with simple_template). In my opinion, it is better for portability if these are as few as possible, even if they are also in-house tools; otherwise they add to the maintenance burden, and can cause problems a few years down the line.

A recent example has been the recent need to upgrade TRUI to python 3, to be able to upgrade the RES; I don't know if the RES was the only tool that depended on TRUI, but it was contemplated for some time to upgrade the RES with reduced capability if TRUI was not made available under python3.

Having said that. I am on the fence on this one.

SVG plots were too slow to render when large.

Fixes #369
jfrost-mo and others added 15 commits February 19, 2024 10:52
The previous code was catching other ValueErrors we wanted to see
the stacktrace for.
importlib.resources.files() works on the current module by default.
The viewer page now adapts to handle both plot sequences, and single
plots to keep the generation code simple. For now I have used a simple
templating library, rather than pulling in Jinja, but it is syntax
compatible if we find we need more templating in future.

Also moved template into a seperate file.

The plot is constructed from the figure to prevent duplicated colourbar.

Fixes #307

Template plots into output page

Needed to get around CORS, but also beneficial for removing a
network round trip.

Make time series recipe more explicit
This allows them to be faster, and more effectively pinpoint problems.
This makes the plotting interface much easier to use, as you no longer
need to worry about the shape of your data. Reasonable defaults have been
chosen for the postage stamp and sequence coordinates (realization and
time respectively) so it will work as expected most of the time.

The existing postage_stamp_contour_plot operator has been marked as
deprecated, but retained, as there are recipes that use it.

Fixes #381

The tests have also been modified:
- Continue to test deprecated operator
- Copy test cube before modifying to prevent other tests being affected
- Assert that plots contains some plots
- Make stamp_coordinate optional
- Test plotting without realization coord
Series makes one think of a line plot, rather than multiple plots.
@jfrost-mo
Copy link
Member Author

jfrost-mo commented Feb 19, 2024

For the web page templating I was initially thinking of using jinja, as that is the definitive python templating library, and would be familiar to people. But that is quite heavy a dependency, and also might give more power than we want by allowing imperitive logic to be put into recipes.

simple_template is something I've actually extracted from some of my personal code, and is a single file pure python templater that just does the variable insertion aspect of jinja. As it is a single file with no dependencies, and is licenced under 0BSD, we could copy it into CSET entirely and remove the external dependency.

@jfrost-mo
Copy link
Member Author

Also are you still having trouble running CSET or is it just time constraints?

As it is a small dependency only needing the standard library, and
is liberally licenced (0BSD) we can just integrate it directly.
@jfrost-mo
Copy link
Member Author

@JorgeBornemann I've now (in b85c034) copied the simple_template module's code inside CSET, and removed the external dependency.

Copy link
Collaborator

@JorgeBornemann JorgeBornemann left a comment

Choose a reason for hiding this comment

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

All further changes look good. Approving.

@jfrost-mo jfrost-mo merged commit 846e09f into main Mar 25, 2024
8 checks passed
@jfrost-mo jfrost-mo deleted the 307_sequential_plot_display branch March 25, 2024 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Displaying sequential plots in output webpage
3 participants