Skip to content

Conversation

@DPeterK
Copy link
Member

@DPeterK DPeterK commented Sep 21, 2016

Here's my proposal for easing the graphics testing issues in Iris. Specifically, this PR proposes...

  • Thinning out most of the graphics tests in iris.tests.plot and iris.tests.quickplot, but retaining the testCoordinatesGiven tests, as well as a few other tests to try and maintain test coverage.
  • Moving iris.tests.plot and iris.tests.quickplot to a new integration tests folder; iris.tests.integration.plotting, with the thinning described in the previous point applied.
  • Moving all common functionality to the plot and quickplot test modules to iris.tests.integration.plotting.__init__.py

This PR (at time of raising) does not:

  • include any changes to ensure all visual tests pass,
  • deal with moving result images around to reflect the changes in test strucure, or
  • deal with removing result images for tests that no longer exist.

These things can be done at a later date if this proposal acquires traction.

@bjlittle @pelson @marqh @lbdreyer @djkirkham your input would be appreciated.

@djkirkham
Copy link
Contributor

Moving iris.tests.plot and iris.tests.quickplot to a new integration tests folder

I'm assuming you mean the files iris/tests/test_plot.py and iris/tests/test_quickplot.py, although the originals haven't been removed.

Is it possible to split this change into two commits to make it easier to see what's changed? One commit with the code changes and a separate one for moving the files.

@DPeterK DPeterK force-pushed the graphics_integration_tests branch from eeeef9f to 0c63a51 Compare September 22, 2016 13:40
@DPeterK
Copy link
Member Author

DPeterK commented Sep 22, 2016

Is it possible to split this change into two commits to make it easier to see what's changed?

It took a while and was fairly circuitous, but I have managed to apply this now...

@DPeterK DPeterK mentioned this pull request Sep 28, 2016
@DPeterK
Copy link
Member Author

DPeterK commented Sep 28, 2016

I've fixed a problem with importing from the __init__ that was meaning all the test classes were being run twice and erroring in half of those cases. I think that the tests still won't pass as we may well have graphical differences in some of the cases.

@DPeterK
Copy link
Member Author

DPeterK commented Sep 28, 2016

@marqh your thoughts on this possible route for helping the graphical tests along would be appreciated!

@DPeterK DPeterK force-pushed the graphics_integration_tests branch from 28cb892 to 444b9ae Compare September 29, 2016 09:21
@marqh
Copy link
Member

marqh commented Sep 29, 2016

@marqh
Copy link
Member

marqh commented Sep 29, 2016

all the tests that I see failing are graphics test which are making images but the tolerances are very large

@dkillick please could you put an image and it's comparison from one of these tests into the ticket so we can see what kind of change we are talking about, perhaps from:
https://travis-ci.org/SciTools/iris/jobs/163442693#L4936
?
ty

@DPeterK
Copy link
Member Author

DPeterK commented Sep 29, 2016

to improve the log output rendering

Done - thanks @marqh.

Of course this need not be a permanent change! It will just make this particular PR a lot more manageable...

@DPeterK DPeterK force-pushed the graphics_integration_tests branch from bb3191e to b565e3b Compare September 29, 2016 11:01
@DPeterK
Copy link
Member Author

DPeterK commented Sep 29, 2016

could you put an image and it's comparison from one of these tests into the ticket so we can see what kind of change we are talking about, perhaps from:
https://travis-ci.org/SciTools/iris/jobs/163442693#L4936

I've added a commit demonstrating the before-after difference for one of the files (the one suggested above). If you take a look at the diff you'll see there is quite a large difference between the two files.

Taking a step back and thinking about this, the large diff makes a lot of sense. The test that produced this particular image was actually a quickplot test masquerading as a plot test. There were a number of examples of these, where qplt was being called within iris.tests.plot. Naturally, as this is bad form I replaced all of the instances of qplt in iris.tests.plot with iplt, but that means that there will be several diffs in this PR caused by the extra annotations that qplt adds no longer being in the result image. I also simplified a number of the tests quite firmly, and this particular test is one such example: I completely removed the extra scatter plot as it seemed an unnecessary complication to the test, and I reckoned that coverage of the visualisation routines won't be affected by its removal.

My thought then is that the new file is appropriate for testing the functionality at the core of this test and that the new image should stand. Would anyone like to counter this?

@pelson
Copy link
Member

pelson commented Oct 4, 2016

@dkillick - I agree with the aspiration to reduce the total number of graphics tests and appreciate the effort that you have gone to to make this a coherent PR, but I'm not sure that anybody will be able to review this change without running the risk of potentially missing a significant reduction in corner case testing...
Is there anything you could do to bridge the gap between where the existing Iris tests are and where you envisage them going? For instance, is it possible to add skip test fixtures to cases that you envisage entirely ditching and proposing those as separate PRs? That would allow us to review each on their own merits, allow us to remove the appropriate result image as we go along and most importantly would allow you to put forwards a restructuring PR without reducing the total number of tests in the same PR.

@pp-mo
Copy link
Member

pp-mo commented Jan 4, 2017

This has gone stale.
@dkillick can you say whether this was entirely superseded by the new imagehash approach ??

@DPeterK
Copy link
Member Author

DPeterK commented Jan 6, 2017

@pp-mo the trick with this PR is in its description: it was my proposal for improving the Iris visual tests. I think it did some useful things by generally improving the code standard of iris/tests/test_plot.py and iris/tests/test_quickplot.py. These are worthwhile changes, but I think I'll close this now given how stale it has got.

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.

5 participants