Skip to content

Support anything that can save as PNG #77

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

Merged
merged 8 commits into from
Feb 20, 2021
Merged

Support anything that can save as PNG #77

merged 8 commits into from
Feb 20, 2021

Conversation

oxinabox
Copy link
Member

@oxinabox oxinabox commented Feb 15, 2021

Including plots.

Closes #62
Alternative to #76

cc @juliohm

@oxinabox
Copy link
Member Author

For 1.0 the problem is that it can't resolve it's test-time dependencies.
Because 1.0 won't reresolve dependencies at test time.
See https://www.oxinabox.net/2021/02/13/Julia-1.6-what-has-changed-since-1.0.html#resolver-willing-to-downgrade-packages-to-install-new-ones-tiered-resolution

For other things, It seems to be because the plot is slightly different.
so I guess need to adjust the sensitivity or find a way to make the plot less sensitive to changes.

Revert "usual actual not raw_actual to decide rendermode"

This reverts commit 9cc6d53.

render actual
@oxinabox
Copy link
Member Author

Failure on 1.0 Ubuntu. idk why. They are indeed slightly different
image

@juliohm
Copy link
Contributor

juliohm commented Feb 17, 2021

Thank you @oxinabox for pushing this forward ❤️

@oxinabox
Copy link
Member Author

@juliohm do you have any ability to dig into what is going on with the heatmap on ubuntu?

@juliohm
Copy link
Contributor

juliohm commented Feb 17, 2021

I am on Manjaro Linux so maybe the issue would reproduce there as well? How can I help? I understand very little about ImageIO and about these possible discrepancies between OSes.

@oxinabox
Copy link
Member Author

I am on Manjaro Linux so maybe the issue would reproduce there as well? How can I help? I understand very little about ImageIO and about these possible discrepancies between OSes.

Can you check out this branch and see if tests pass for you locally?

@juliohm
Copy link
Contributor

juliohm commented Feb 17, 2021

I get a couple of warnings but all the tests pass.

image

@oxinabox
Copy link
Member Author

hmmm, Not sure what to do here.
Maybe mark it conditionally broken and open an issue?
Maybe we can up the tolerance? idk how to up tolerence

@juliohm
Copy link
Contributor

juliohm commented Feb 17, 2021

I had the same question in the packages where I am using ReferenceTests.jl How to increase tolerance? The current threshold with PSNR is not very intuitive, at least for me, and guidelines are appreciated. Perhaps increase the default tolerance in ReferenceTests.jl would also be a good idea.

Any thoughts on that @johnnychen94 ? It would be nice to fix this OS-specific issue, perhaps just increasing tolerance for now.

Copy link
Member

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

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

A nice and clear solution!

@oxinabox
Copy link
Member Author

oxinabox commented Feb 19, 2021

Warning: test fails because PSNR -22.433345794677734 < 15 for the heat map

@oxinabox
Copy link
Member Author

OK, I have spun up a linux VM and replicated the failure, not that i can replace it, fixing it should be easier

@oxinabox
Copy link
Member Author

Confirmed it is Plots/GR actually making a clearly different output on linux.

Linux

image

Mac / Windows

image

@oxinabox
Copy link
Member Author

This is JuliaPlots/Plots.jl#2127
Now that i know it is a know bug, I feel ok just disabling this test til it is fixed.
It's not our problem to fix.

If there are no objections I will merge this tomorrow

@oxinabox oxinabox merged commit 7ad5b95 into master Feb 20, 2021
@oxinabox oxinabox deleted the ox/plot branch February 20, 2021 13:35
@oxinabox oxinabox mentioned this pull request Feb 20, 2021
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.

@plottest
3 participants