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

Nose testing image comparison: label which image is which? #4954

Closed
nastasha-w opened this issue Jul 23, 2024 · 3 comments
Closed

Nose testing image comparison: label which image is which? #4954

nastasha-w opened this issue Jul 23, 2024 · 3 comments
Labels
tests: running tests Issues with the test setup
Milestone

Comments

@nastasha-w
Copy link
Contributor

Enhancement suggestion

Problem to solve

In looking into the image comparison tests for PR #4939, @neutrinoceros and I were both confused about which nose test image was produced with the pull request branch, and which was the standard it was being compared against. This caused some real confusion when we thought the images looked worse after the suggested bug fix in that PR. It would be great if the images (or their file names) were labelled to avoid such confusion in the future.

Example of the problem

I'm not sure how long this specific page will remain accessible, but on for example this page, the images made with the two yt versions are labelled as
'tmpgibl3qlg.png' and
'tmpvym0ts42.png',
which gives no indication of which image is the standard/baseline and which is the version produced after the changes proposed in the PR. There is also a difference image provided, called 'tmpvym0ts42-failed-diff.png', but this image does not seem to be very helpful either: it still doesn't indicate what the baseline is, and it doesn't seem to show which image was subtracted from which. (As far as I can tell, it is a direct difference of the images, not a plot showing the differences in the plotted data. I'm sure it's useful for telling if a difference is due to data changes or plotting changes.)

I reached the page with those images from PR #4939 by clicking the 'details' link (here) in the 'Full testsuite (nose)' row of the tests section of the PR. This section is at the bottom of the main PR page, below the comments. From there, it's just the first link in the 'test results' section or page.

Suggestion

Renaming the images to something like 'baseline_<unique numbers/letters>.png' and 'proposed_<unique numbers/letters>.png' or 'thisPR_<unique numbers/letters>.png' would help a lot. I'm not familiar enough with nose to know if that is actually possible though.

Version Information

  • nose test suite used in yt on 2024-07-21
  • viewed on github.com
  • browser: Chrome version 126.0.6478.183
  • OS: macOS Sonoma 14.5, running on a 2021 M1 mac.
@neutrinoceros
Copy link
Member

If I remember correctly this used to be clearer before Jenkins went through some visual overhaul, but I agree that it's now left for the reader to guess that the first image is the baseline and the second is the new one.

I think the function you want to modify is

def dump_images(new_result, old_result, decimals=10):
tmpfd, old_image = tempfile.mkstemp(suffix=".png")
os.close(tmpfd)
tmpfd, new_image = tempfile.mkstemp(suffix=".png")
os.close(tmpfd)
image_writer.write_projection(new_result, new_image)
image_writer.write_projection(old_result, old_image)
results = compare_images(old_image, new_image, 10 ** (-decimals))
if results is not None:
tempfiles = [
line.strip() for line in results.split("\n") if line.endswith(".png")
]
for fn in tempfiles:
sys.stderr.write(f"\n[[ATTACHMENT|{fn}]]")
sys.stderr.write("\n")

And it probably suffices to use the prefix argument in tempfile.mkstemp

@nastasha-w
Copy link
Contributor Author

I'll merge this with PR #4939 to check if this changes the file names as intended.

@chrishavlin
Copy link
Contributor

Closed by #4956

@neutrinoceros neutrinoceros added this to the 4.4.0 milestone Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests: running tests Issues with the test setup
Projects
None yet
Development

No branches or pull requests

3 participants