Skip to content

Switch to using hash-based figure comparison - #13596

Merged
astrofrog merged 9 commits into
astropy:mainfrom
astrofrog:figure-hash-tests
Sep 8, 2022
Merged

Switch to using hash-based figure comparison#13596
astrofrog merged 9 commits into
astropy:mainfrom
astrofrog:figure-hash-tests

Conversation

@astrofrog

@astrofrog astrofrog commented Sep 5, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

This switches the figure testing to use hash-based figure comparison, as described here and as done in sunpy. The idea is that we keep the latest expected hashes for each figure and tox environment, and if a figure test fails we can get a nice summary page here in the PR and see the difference and choose whether the changes make sense - if they do we just update the hash. Once the PR is merged, the baseline figures are then updated in astropy-data. This means we would no longer need to update astropy-data before a PR is merged which would be a big win as that is usually a barrier to contribution and has resulted in several languishing PRs.

To do:

  • Determine whether we want to split up the hash library files by e.g. freetype/libpng version as done in SunPy. This does add some complexity and might not be needed for now, so we might be able to defer until a separate PR. won't do for now, can cross that bridge if we need
  • Add the infrastructure to automatically push the reference images to astropy-data upon merging
  • Update any URLs to point to astropy instead of my fork.
  • Add the hash libraries for the current tox environments being tested.
  • Update the developer docs
  • Remove any temporary commits

@github-actions

github-actions Bot commented Sep 5, 2022

Copy link
Copy Markdown
Contributor

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

@astrofrog astrofrog mentioned this pull request Sep 5, 2022
Closed
@Cadair

Cadair commented Sep 5, 2022

Copy link
Copy Markdown
Member

It's so beautiful: https://output.circle-artifacts.com/output/job/bc9819c6-2fe1-4559-82af-2ee179652757/artifacts/0/results/fig_comparison.html

@astrofrog
astrofrog force-pushed the figure-hash-tests branch 2 times, most recently from 1206659 to 066cc55 Compare September 5, 2022 22:58
@astrofrog
astrofrog marked this pull request as ready for review September 5, 2022 22:58
@astrofrog
astrofrog requested a review from Cadair as a code owner September 5, 2022 22:58
@astrofrog
astrofrog requested review from bsipocz, pllim and saimn September 5, 2022 23:01
@astrofrog astrofrog added this to the v5.2 milestone Sep 5, 2022
bsipocz
bsipocz reviewed Sep 6, 2022
View reviewed changes

@bsipocz bsipocz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suppose that the original argument against the hash-based approach is not relevant anymore, namely that it was very sensitive to non-important tiny differences that fit within the tolerance range? If that's the case, then this PR indeed provides a huge improvement of the status quo.

@pllim

pllim commented Sep 6, 2022

Copy link
Copy Markdown
Member

RTD warning needs fixing:

docs/development/testguide.rst:804: WARNING: Unknown target name: "https://github.com/astropy/astropy-figure-tests"

pllim
pllim reviewed Sep 6, 2022
View reviewed changes
Comment thread docs/development/testguide.rst Outdated
@astrofrog

astrofrog commented Sep 6, 2022

Copy link
Copy Markdown
Member Author

@bsipocz indeed we actually set tolerance=0 now for all our image tests so we were being ultra strict anyway.

@astrofrog @pllim
Fix link
70139e2 
Co-authored-by: P. L. Lim <[email protected]>
@Cadair

Cadair commented Sep 7, 2022

Copy link
Copy Markdown
Member

This is a bit of an aside but:

it was very sensitive to non-important tiny differences that fit within the tolerance range

It still is, these hashes are still cryptographic hashes of the whole png file, so one byte of difference and the test fails.

However, this issue is mitigated by a few key things:

  • Firstly, a strict control of the environment on any linux machine is possible by using tox alone, so I can easily regenerate the hashes on my local machine. (Note, it is not consistent over OSes, so you have to use linux to generate the hashes and run the tests) This property isn't that useful for devs, but it is very useful for being able to change CI providers.
  • Secondly, we haven't done away with the comparison images, so when a test fails the new image is compared to the last one and it's obvious if the difference in the hashes is something trivial vs a real change in behaviour.
  • Finally, because we haven't done away with the images, but automated their generation we actually have a much better review workflow now. To make the tests pass all you need to do is update a json file in this repo. That's easy enough for most contributors to do themselves. The really neat thing though is that after you have updated the json the tests will pass, but the image comparison HTML summary page will show you the differences between the last build on main and the PR. This let's reviewers see a summary of the differences in the images with the tests passing.

Cadair
Cadair reviewed Sep 7, 2022
View reviewed changes
Comment thread .circleci/config.yml Outdated
Comment thread .circleci/config.yml Outdated
Comment thread astropy/visualization/wcsaxes/tests/test_frame.py
Comment thread docs/development/testguide.rst Outdated
Comment thread docs/development/testguide.rst Outdated
Cadair
Cadair reviewed Sep 8, 2022
View reviewed changes
Comment thread tox.ini Outdated
# For coverage, we need to pass extra options to the C compiler
cov: CFLAGS = --coverage -fno-inline-functions -O0
image: MPLFLAGS = --mpl
image: MPLFLAGS = -m "mpl_image_compare" --mpl --mpl-generate-summary=html --mpl-results-path={toxinidir}/results --mpl-hash-library={toxinidir}/astropy/tests/figures/{envname}.json --mpl-baseline-path=http://data.astropy.org/testing/astropy/figures/{envname}/ --remote-data=astropy -W ignore::DeprecationWarning

@Cadair Cadair Sep 8, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do we have the old data repo here and not the new one? If there aren't any figures to compare it against, it will still work.

@astrofrog astrofrog Sep 8, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oops, mistake

@astrofrog
astrofrog merged commit 68dce23 into astropy:main Sep 8, 2022
@astrofrog

astrofrog commented Sep 8, 2022

Copy link
Copy Markdown
Member Author

I will need to backport this otherwise we won't be able to backport bug fixes for WCSAxes

@astrofrog

astrofrog commented Sep 8, 2022

Copy link
Copy Markdown
Member Author

@meeseeksdev backport to v5.1.x

@astrofrog

astrofrog commented Sep 8, 2022

Copy link
Copy Markdown
Member Author

@meeseeksdev backport to v5.0.x

@meeseeksmachine meeseeksmachine mentioned this pull request Sep 8, 2022
Merged
meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request Sep 8, 2022
@astrofrog @meeseeksmachine
Backport PR astropy#13596: Switch to using hash-based figure comparison
cef3f58
@meeseeksmachine meeseeksmachine mentioned this pull request Sep 8, 2022
Merged
meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request Sep 8, 2022
@astrofrog @meeseeksmachine
Backport PR astropy#13596: Switch to using hash-based figure comparison
83b3c73
Cadair added a commit that referenced this pull request Sep 8, 2022
@Cadair
Merge pull request #13610 from meeseeksmachine/auto-backport-of-pr-13…
5b9e22b 
…596-on-v5.1.x

Backport PR #13596 on branch v5.1.x (Switch to using hash-based figure comparison)
Cadair added a commit that referenced this pull request Sep 8, 2022
@Cadair
Merge pull request #13611 from meeseeksmachine/auto-backport-of-pr-13…
5a53003 
…596-on-v5.0.x

Backport PR #13596 on branch v5.0.x (Switch to using hash-based figure comparison)
This was referenced Jan 17, 2023
Closed
Closed
@pllim pllim modified the milestones: v5.2, v5.0.6 Feb 9, 2023
@pllim pllim mentioned this pull request Feb 9, 2023
Closed
@pllim pllim mentioned this pull request Nov 4, 2024
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pllim pllim pllim left review comments
@bsipocz bsipocz bsipocz left review comments
@Cadair Cadair Cadair approved these changes
@saimn saimn Awaiting requested review from saimn

Assignees

No one assigned

Labels

Docs no-changelog-entry-needed testing visualization.wcsaxes

Projects

None yet

Milestone

v5.0.6

Development

Successfully merging this pull request may close these issues.

4 participants

@astrofrog @Cadair @pllim @bsipocz