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

Enable pipeline to display multiple plots per script #26

Merged
merged 2 commits into from
Aug 21, 2021

Conversation

EvgeniiChaikin
Copy link
Collaborator

@EvgeniiChaikin EvgeniiChaikin commented Aug 20, 2021

Enable pipeline to display multiple plots per script

Example ( how the config may look like for a script with multiple plots)

scripts:
  - filename: scripts/density_temperature.py
    caption:
      - Caption for plot 1.
      - Caption for plot 2.
      - Caption for plot 3.
    output_file: [density_temperature1.png, density_temperature2.png, density_temperature3.png]
    section: Density-Temperature
    title:
      - Density-Temperature plot 1
      - Density-Temperature plot 2
      - Density-Temperature plot 3

Basic logic in the implementation:

  • If a script makes several plots then output_file must be a list of strings. If so, then caption and title must also be lists of strings (of the same size) because each plot must have its own title and caption.
  • Section must be a string, not a list of strings. Because we do not expect scripts to make plots so different that they belong to different sections.

@EvgeniiChaikin EvgeniiChaikin changed the title Enable pipeline display multiple plots per script Enable pipeline to display multiple plots per script Aug 20, 2021
@EvgeniiChaikin
Copy link
Collaborator Author

What I don't like about the current implementation is that it might not be super clear to users which parameters in the script config are now allowed to be lists, and which are not allowed

We also have an optional bool parameter show_on_webpage, which currently needs to be specified per script, not per plot in the script. Not sure if we want to change it, it is definitely worth to at least discuss it.

Copy link
Member

@bwvdnbro bwvdnbro left a comment

Choose a reason for hiding this comment

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

Looks good to me. I'm still running a test to see if the new feature works for my use case, but I don't see why it would not.

@bwvdnbro
Copy link
Member

Looks good to me. I'm still running a test to see if the new feature works for my use case, but I don't see why it would not.

I just realised that sounds like a general template for a review. Insert your own new feature and use case and you are good to go. I swear I did actually look at the code.

Copy link
Member

@bwvdnbro bwvdnbro left a comment

Choose a reason for hiding this comment

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

Works beautifully. Thanks!

@EvgeniiChaikin
Copy link
Collaborator Author

Nice! Thanks for the feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants