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

Output figure diff in test figure page #2681

Merged
merged 7 commits into from Oct 10, 2018

Conversation

Projects
None yet
4 participants
@dstansby
Copy link
Contributor

commented Jul 4, 2018

Requires remote data to download the baseline images to generate a diff, but I'm not sure if there's a way to avoid that?

@pep8speaks

This comment has been minimized.

Copy link

commented Jul 4, 2018

Hello @dstansby! Thanks for updating the PR.

Comment last updated on October 01, 2018 at 19:50 Hours UTC
@sunpy-bot

This comment has been minimized.

Copy link

commented Jul 4, 2018

Thanks for the pull request @dstansby! Everything looks great!

@nabobalis

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2018

How would you feel about switching the other of the plots? Either old, new, diff or old, diff, new?

@dstansby

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2018

Happy to change the order, maybe old diff new works best.

@dstansby

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2018

As it stands in this PR doing python setup.py test --figure-only won't do anything because it now needs --online to get the baseline images, maybe I should make generating the diff contingent on --onilne being passed so figure tests can still be run offline?

@nabobalis

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2018

Yes, I do think we want to be able to at least run the figure tests offline.

@dstansby dstansby referenced this pull request Jul 5, 2018

Open

Comparing images from image tests #2506

4 of 5 tasks complete
@dstansby

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2018

@Cadair is there a way to work out when running the tests if the --online option has been passed to pytest? I want to only do the image comparison if that's the case.

@Cadair

This comment has been minimized.

Copy link
Member

commented Jul 11, 2018

@dstansby I mean pytest will know, where do you want to do the check?

@Cadair

This comment has been minimized.

Copy link
Member

commented Jul 17, 2018

@dstansby I worked out how to do it:

def pytest_unconfigure(config):
    try:
        rd = config.getoption('remote_data')
    except ValueError:
        rd = 'none'
    remotedata = rd == 'any'

However, I think it's silly that we have to specify --online to be able to generate the image comparisons, so I hacked in a way to enable it again, see PR dstansby#1

@Cadair

This comment has been minimized.

Copy link
Member

commented Jul 17, 2018

A general question about this: What happens if the baseline image hasn't been uploaded to the repo yet? It looks like in that case it might explode in quite a few places?

@nabobalis nabobalis added this to the 1.0 milestone Jul 18, 2018

@dstansby dstansby force-pushed the dstansby:figure-diff branch from b2bc698 to 69d452a Jul 24, 2018

@dstansby

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2018

I've added some code to handle baseline images not being available.

@@ -23,7 +23,7 @@ image-run: &image-tests
command: |
conda env list
source activate sunpy-figure-tests-3.6
python setup.py test --figure-only --coverage
python setup.py test --figure-only --coverage --online

This comment has been minimized.

Copy link
@Cadair

Cadair Jul 25, 2018

Member

I don't think this is needed any more.

@Cadair

This comment has been minimized.

Copy link
Member

commented Jul 25, 2018

I think this is basically good to go. It would be good to have a test or two for the HTML page comparison function?

@Cadair

This comment has been minimized.

Copy link
Member

commented Jul 25, 2018

oh also a changelog

@dstansby

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2018

@Cadair do your changes mean that the --online flag isn't really respected now? (ie. if you don't pass --online, running tests will still connect to the internet to do the figure comparison)

@Cadair

This comment has been minimized.

Copy link
Member

commented Jul 26, 2018

Yes, it will now connect to the internet irrespective of the online flag. Specifically for building the comparison page. (So the building should fail gracefully if the machine is offline)

@dstansby

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2018

Hmm, but that seems a bit dishonest, if I don't pass the --online flag then I would expect my machine not to try and connect to the internet. There must be a way to detect if the --online flag has been passed as a way of deciding whether to fetch the baseline images.

@Cadair

This comment has been minimized.

Copy link
Member

commented Jul 26, 2018

The idea of the --online flag is really to run the tests which are marked as such. I don't really consider the generation of the summary page part of the test run. Also, I don't want to have to run the online tests just to build the figure comparison page (i.e. I might just want to see the comparisons for offline tests).

Can we not just have the comparison builder attempt to download the images and fail gracefully, is this not the same effect as respecting --online without entangling ourselves with the selection of the tests to run?

@dstansby

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2018

👍 okay, I'm happy for this to go in as is. This is a gentle nudge that now Matplotlib 3.0 has just been released it would be nice to have this PR merged so we can see the differences in Sunpy test images with the new release.

@dstansby dstansby force-pushed the dstansby:figure-diff branch from 69d452a to 64081bb Oct 1, 2018

@nabobalis

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2018

@dstansby Any chance of a changelog entry? I think this PR does warrant one.

Also do we want to still add --online to the CI command?

@Cadair

Cadair approved these changes Oct 10, 2018

@nabobalis nabobalis merged commit a68a754 into sunpy:master Oct 10, 2018

7 of 8 checks passed

codecov/patch 17.39% of diff hit (target 74.22%)
Details
ci/circleci: egg-info-36 Your tests passed on CircleCI!
Details
ci/circleci: figure-tests-36 Your tests passed on CircleCI!
Details
ci/circleci: html-docs Your tests passed on CircleCI!
Details
codecov/project 85.24% (+11.01%) compared to 085093e
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
sunpy-bot All checks passed

@dstansby dstansby deleted the dstansby:figure-diff branch Oct 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.