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

Add line plots #325

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

Add line plots #325

wants to merge 8 commits into from

Conversation

connoraird
Copy link
Collaborator

Adding the option to plot line charts.

Copy link
Collaborator

@pineapple-cat pineapple-cat left a comment

Choose a reason for hiding this comment

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

Looking good so far! Don't forget to update any unit test that has a config with "plot_type": "generic", at the start so we can make sure the CI is passing cleanly.

post-processing/config_handler.py Outdated Show resolved Hide resolved
post-processing/streamlit_post_processing.py Outdated Show resolved Hide resolved
post-processing/plot_handler.py Outdated Show resolved Hide resolved
post-processing/plot_handler.py Outdated Show resolved Hide resolved
@pineapple-cat
Copy link
Collaborator

pineapple-cat commented May 29, 2024

This has remained undocumented thus far, but I've been using flake8 with a 120 char/line limit for style enforcement. Perhaps it's time to add a linter to the CI...

Edit: With regards to the current failing checks, pytest passes for me locally and nothing I added should have broken the CI. I think I had a similar issue with Kaan's PR, but can't quite remember how we solved it.

Copy link
Collaborator

@pineapple-cat pineapple-cat left a comment

Choose a reason for hiding this comment

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

Good work, very close to being done I think. I've just made a few more notes and noticed that a good final touch to add would be to update the README config structure and config template sections to account for the new custom axis ranges.

post-processing/streamlit_post_processing.py Outdated Show resolved Hide resolved
post-processing/streamlit_post_processing.py Outdated Show resolved Hide resolved
post-processing/streamlit_post_processing.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@pineapple-cat pineapple-cat left a comment

Choose a reason for hiding this comment

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

I think I'm more or less happy with the way this PR looks now. The important functionality is all there and I don't see any more major bugs that need to be fixed. Pytest is still passing locally.

Once this gets merged, I think any of the following bits that aren't implemented here should collectively belong to some sort of line plot enhancement issue:

  • Update README config structure + template sections to account for new custom axis ranges.
  • Implement custom axis ranges for bar charts OR hide range selectors. (For now could also add tooltips to range selectors with this information.)
  • Allow use of custom values for only one of min or max.

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