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

Allow interactively inspect plots #208

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

Conversation

DanielYang59
Copy link
Collaborator

@DanielYang59 DanielYang59 commented Sep 25, 2024

Summary

Allow interactively inspect plots.

Objective:
Add an interactive inspection tool to prompt the user to inspect a plot, this would only run locally and skip CI workflow.

Rationale:
It's really hard to automate tests for plotters, we could assert some elements (title, number of subplots and such), but it's hard to ensure the overall figure is in expected shape. Therefore we might be better off insert some interactive checkers which would only run locally anyway. Such that if developers want to inspect some plotters, they could run that particular unit test locally.

Currently I mostly use the "make_assets" scripts to inspect plots, but it's largely limited to certain "styles". Also it's not quite reusable if I wish to inspect some other styles.

@DanielYang59 DanielYang59 self-assigned this Sep 25, 2024
@DanielYang59 DanielYang59 added testing Test all the things matplotlib Concerning matplotlib-powered functions labels Sep 25, 2024
@janosh
Copy link
Owner

janosh commented Sep 25, 2024

the way this is usually solved is with reference files and doing pixel-averaged distance between those and newly generated plots. see matplotlib.testing.compare.compare_images

i briefly looked into doing that in #22 but seemed too brittle to pursue. also a bit annoying having to have a lot of references files in git history, esp. since they i suspect they need to be bitmapped. could maybe use the SVG assets and convert them to PNGs on the fly but that would slow down tests.

@DanielYang59
Copy link
Collaborator Author

DanielYang59 commented Sep 25, 2024

I guess my option is more naive: "just ask the user to inspect the figures themselve to see if it's broken", with a prompt like please check if the colorbar is displayed properly.

I intend to make this optional on top of the regular test workflow (only trigger in non-CI environment), such that if we did some major change to the code base, we run these tests locally to see if there is any major breakage.

I would give it a spin first and see how everything goes :)

@DanielYang59
Copy link
Collaborator Author

DanielYang59 commented Sep 30, 2024

I would close this to merge this into #200 as that's a better place to test this function out.

Update: tested out in #200 but doesn't seem to work well, interactively showing seems to break the layout of some plots. I might go back to this later.

@DanielYang59 DanielYang59 deleted the real-interactive-unit-test branch September 30, 2024 07:30
@DanielYang59 DanielYang59 restored the real-interactive-unit-test branch October 6, 2024 07:26
@DanielYang59 DanielYang59 reopened this Oct 6, 2024
@janosh janosh force-pushed the main branch 2 times, most recently from 889588d to 3d818a1 Compare October 16, 2024 21:53
@janosh janosh force-pushed the main branch 6 times, most recently from a4619c1 to cbed2dc Compare December 12, 2024 14:26
@janosh
Copy link
Owner

janosh commented Dec 14, 2024

is this PR still on the to do list or could it be closed?

* density_scatter_plotly add keyword colorbar_kwargs for customizing colorbar

- increase tolerance in `get_image_sites` in `helpers.py` from 0.02 to 0.03
- add new test for colorbar customization in `test_scatter.py`

* add plot_hetero_diatomic_curves.py with ptable and 3D line plots

set Atoms(pbc=False) in mace_pair_repulsion.py for safety (already the default)

* drop windows CI
@DanielYang59
Copy link
Collaborator Author

is this PR still on the to do list or could it be closed?

Sorry for the lack of response and progress, I cleared most of my TODO list from the MP side so I would get my hands on these by next week ;)

@DanielYang59 DanielYang59 force-pushed the real-interactive-unit-test branch 2 times, most recently from 3014528 to 8144fbe Compare December 18, 2024 07:44
@DanielYang59 DanielYang59 force-pushed the real-interactive-unit-test branch from 8144fbe to 5079a56 Compare December 18, 2024 07:53
@DanielYang59 DanielYang59 force-pushed the real-interactive-unit-test branch from 6225dd0 to 4f795c7 Compare December 18, 2024 08:46
@DanielYang59
Copy link
Collaborator Author

DanielYang59 commented Dec 21, 2024

I'm happy with current functionality so far, it displays the figure optionally when invoked interactively, and doesn't require much (if any) code change in our current test code.

However there may be a few quality-of-life improvement possible:

  • Current implementation saves the figure to a temporary location and re-open it (otherwise the display style might change with window size and so on), this sounds a bit stupid, wondering if there's a way to display the figure directly and show the same result as saved figure?
  • pytest would capture output by default, and to use this we have to explicitly pytest -s test_script, I tried but didn't manage to release the capture from inside the interactive_check helper function.
  • displayed figures tend to be larger than I like it to be, I tried to scale it by monitor size (say 25% of the screen and at top-center) but didn't have much luck so far. I previously found a SO thread saying the API to get display size is backend-dependent, but I would have another try

@DanielYang59
Copy link
Collaborator Author

DanielYang59 commented Jan 4, 2025

Just I noticed another pytest plugin from matplotlib to compare images by their RMS:

pytest-mpl is a pytest plugin to facilitate image comparison for Matplotlib figures.

For each figure to test, an image is generated and then subtracted from an existing reference image. If the RMS of the residual is larger than a user-specified tolerance, the test will fail. Alternatively, the generated image can be hashed and compared to an expected value.

For more information, see the pytest-mpl documentation.

Though I'm hoping their functionalities to complement one another (the pytest add-on for automated CI pipeline), and this for easier manual inspection (as this don't need to save a copy of the target figure)

@@ -70,7 +70,7 @@ build-backend = "setuptools.build_meta"

[tool.pytest.ini_options]
testpaths = ["tests"]
addopts = "-p no:warnings"
addopts = "-s -p no:warnings"
Copy link
Collaborator Author

@DanielYang59 DanielYang59 Jan 5, 2025

Choose a reason for hiding this comment

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

Admittedly this is not the best option (to globally un-capture output) as I was hoping this could be integrated without any code change to our current test framework, but I didn't find another way around this issue.

The most related might be the capsys fixture but again I didn't manage to integrate this fixture into this helper function.

@janosh Not sure if you would be happy with adding this option to globally release pytest output capture (though I would assume the side effect to be limited as we aren't inserting much print and so on), and save the trouble to pass -s every time we need to use this helper function. But I would be happy with either as passing a -s would not be too much effort as manual inspection shouldn't be something super frequently done


Also for the other two possible improvements mentioned in #208 (comment), unfortunately I didn't find better solutions (the display size getter seems heavily OS-specific). Not sure if you have any comment?

Is there a way to open a figure with the same style as opening a saved one? I was taking the "saved" one as reference as it's more stable (and better looking in my case, as the directly shown one tend to overlap):

Happy coding 2025 in any case :)

@DanielYang59 DanielYang59 marked this pull request as ready for review January 5, 2025 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
matplotlib Concerning matplotlib-powered functions testing Test all the things
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants