search

matplotlib / pytest-mpl

A pytest plugin to facilitate image comparison for Matplotlib figures
Other
244 stars 47 forks source link

result_image redundancy #153

Open bjlittle opened 2 years ago

bjlittle commented 2 years ago

In the plugin.ImageComparison.compare_image_to_hash_library method, during hybrid-mode, the result_image appears to be unnecessarily copied into the summary dictionary from the outcome of plugin.ImageComparison.compare_image_to_baseline:

https://github.com/matplotlib/pytest-mpl/blob/e3876180847c2d04aeda1982372074998a7f8716/pytest_mpl/plugin.py#L571-L573

i.e., the summary['result_image'] has already been correctly set within plugin.ImageComparison.compare_image_to_hash:

https://github.com/matplotlib/pytest-mpl/blob/e3876180847c2d04aeda1982372074998a7f8716/pytest_mpl/plugin.py#L551-L554

If this is the case, are you happy for me to remove this behaviour?

dopplershift commented 2 years ago

Actually, it looks like compare_image_to_baseline duplicates the entire 3-line block of setting up the result image, which doesn't seem necessary at all?

Cadair commented 2 years ago

I suspect that it could be tidied up, feel free to go for it.

I think it was @ConorMacBride who touched this section last?

ConorMacBride commented 2 years ago

compare_image_to_baseline runs:

https://github.com/matplotlib/pytest-mpl/blob/e3876180847c2d04aeda1982372074998a7f8716/pytest_mpl/plugin.py#L445-L447

So technically the result_image file generated in compare_image_to_hash_library is different to the new one generated by compare_image_to_baseline, although it should be at the same path. I just set it to copy the result_image to be safe, as both methods define their own (but equal) path.

These three lines are duplicated because the plugin can do baseline image comparison, hash library comparison or both (hybrid mode). But to prevent saving twice, maybe they should be moved to a separate method which only saves if it doesn't already exist.