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

Adds figure test capability #38

Merged
merged 9 commits into from
Jul 21, 2021
Merged

Adds figure test capability #38

merged 9 commits into from
Jul 21, 2021

Conversation

jeffreypaul15
Copy link
Collaborator

@jeffreypaul15 jeffreypaul15 commented Jul 10, 2021

The figure tests can be performed using verify_cache_image() method in test_plotting.
3 command line arguments can be passed

  • reset_image_cache : rests the image_cache directory with all the new figures
  • add_image_cache : option I added to make sure we don't mistakenly add a new figure unless this is specified to be True
  • ignore_image_cache : ignores the image cache, doesn't perform comparing of the images.

The comparison happens through pyvista.compare_images() and the a threshold is set for the allowed error
(IMAGE_REGRESSION_ERROR >= 500 and warning at >=200).
High variance tests allow for a higher threshold with IMAGE_REGRESSION_ERROR >= 1000, but we haven't used this as all of our tests fall in the normal range (probably because we're not changing lighting conditions). I've left this in as maybe some of our tests might fall in this range.

Usage -

plotter.show(before_close_callback=verify_cache_image)
# plotter being PyvistaPlotter not SunpyPlotter

@jeffreypaul15 jeffreypaul15 marked this pull request as draft July 10, 2021 18:03
@jeffreypaul15
Copy link
Collaborator Author

That's odd, it seems to be working fine on the azure pipelines

@jeffreypaul15 jeffreypaul15 marked this pull request as ready for review July 11, 2021 17:46
tox.ini Outdated Show resolved Hide resolved
@jeffreypaul15 jeffreypaul15 changed the title Tests Adds figure test capability Jul 12, 2021
@jeffreypaul15
Copy link
Collaborator Author

jeffreypaul15 commented Jul 12, 2021

There seems to be some issue with the tests working on pytest and not working with tox.
The image is giving me an error of 12145 but after taking a look at the images, they seem to be the exact same though.

@dstansby
Copy link
Member

So I think we just need to either reduce the tolerance of the plot_quadrangle test, or upload a new reference image. I'm not sure how we can get at the image generated on Azure pipelines though...

setup.cfg Outdated Show resolved Hide resolved
@jeffreypaul15
Copy link
Collaborator Author

jeffreypaul15 commented Jul 16, 2021

@nabobalis @dstansby Would it be better to test each mesh separately or test them plotted a map.
Also would it be okay to have a test for each example to make sure that they're producing the right output?

@dstansby
Copy link
Member

In general it would be good to keep the number of figure tests to a minimum, as that then reduces the number of test images we have to store (and potentially change in the future). We can test multiple features by adding them to the same test, and just making sure they're in view of the camera (ie. visible in the saved image).

At the moment, I think we should have two tests:

  • One for everything except pfsspy field lines
  • One for pfsspy field lines (including the map used to generate them)

@dstansby
Copy link
Member

Also, since I've just merged #41 you'll need to rebase this to make sure the tests don't error due to any new warnings.

@jeffreypaul15
Copy link
Collaborator Author

In general it would be good to keep the number of figure tests to a minimum, as that then reduces the number of test images we have to store (and potentially change in the future). We can test multiple features by adding them to the same test, and just making sure they're in view of the camera (ie. visible in the saved image).

At the moment, I think we should have two tests:

* One for everything except `pfsspy` field lines

* One for `pfsspy` field lines (including the map used to generate them)

Alright, I shall update them asap.

@nabobalis
Copy link
Contributor

SQUASH MERGE THIS

@nabobalis nabobalis merged commit 6b79400 into sunpy:main Jul 21, 2021
@jeffreypaul15 jeffreypaul15 mentioned this pull request Jul 21, 2021
6 tasks
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.

3 participants