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

migrate to cirrus-ci and support mpl 3.4.1 #39

Merged
merged 1 commit into from
Apr 9, 2021

Conversation

bjlittle
Copy link
Member

@bjlittle bjlittle commented Apr 8, 2021

The PR migrates the repo from travis-ci to cirrus-ci.

Additionally, it also:

  • adds a pre-commit git hook
  • rationalises the package requirements into requirements.yml
  • flake8 lints the code
  • blackifys the code
  • adds the require phash images to support mpl 3.4.1

i.e., generally shows some love to test-iris-imagehash 👍

@bjlittle bjlittle force-pushed the support-mpl-341 branch 2 times, most recently from 4718e7a to ae83824 Compare April 8, 2021 21:33
@bjlittle
Copy link
Member Author

bjlittle commented Apr 8, 2021

The additional image files are a collection of the usual culprits:

  • minor anti-aliasing differences
  • font and tick changes
  • axes pixel changes
  • pixel banding between contour levels i.e., the transition from one contour colour level to the next
  • some contour rendering differences, but minor broad-brush pixel shifting and differences

Nothing that I'd constitute as a macroscopic visual change i.e., changes the scientific interpretation by the viewer. To the eye, the changes are imperceptible.

@bjlittle bjlittle requested a review from trexfeathers April 9, 2021 06:39
@bjlittle
Copy link
Member Author

bjlittle commented Apr 9, 2021

Ping @jamesp

@jamesp
Copy link
Member

jamesp commented Apr 9, 2021

Go mambaforge! 👍

@bjlittle
Copy link
Member Author

bjlittle commented Apr 9, 2021

It seems somewhat total overkill for this repo... but, what the heck 😉

It's a reasonable pattern to adopt and borrow from the collaboration with https://github.com/pletzer/mint/blob/master/.cirrus.yml

If the glove fits... 😉

@jamesp
Copy link
Member

jamesp commented Apr 9, 2021

awesome 🚀

@trexfeathers
Copy link
Contributor

It seems somewhat total overkill for this repo

I whole-heartedly agree. Was expecting this to a 5min review tops 🙄

@bjlittle
Copy link
Member Author

bjlittle commented Apr 9, 2021

@trexfeathers My point about overkill, is that the minimum you need to do is quite a lot relative to the content of the repo.

I have added extra "goodness", but that is simply plumbing for best practice i.e., black, flake8 and pre-commit all of which are common patterns elsewhere through out SciTools

@jamesp
Copy link
Member

jamesp commented Apr 9, 2021

Ok, ci looks reasonable to me and the automated tests are passing which is good 👍

I've done some spot checks of the image hashes you're including and they match what I get with iris master HEAD and matplotlib 3.4.1.

The only one that looks materially different on my instance is: result-iris.tests.test_mapping.TestLimitedAreaCube.test_grid.0.png

This corresponds to https://github.com/SciTools/test-iris-imagehash/blob/a980a1b1fcbf1b6a747c74b959264a2e9c413062/images/v4/fa1585e885ea7a1785fa7a157a177a017a1585e817a885ea85e86a1785fa7a17.png in this PR

extract of imagerepo.json below suggests this grid should have colour structure:
"iris.tests.test_mapping.TestLimitedAreaCube.test_grid.0": [
"https://scitools.github.io/test-iris-imagehash/images/v4/bf80e2b1c17f1d0ac4f7c8d739a637202749699b6bb3ce3666e4b048944d9d89.png",
"https://scitools.github.io/test-iris-imagehash/images/v4/bf80e2f1c17f1d0ac457c8d619a637213749699b6bb34e3666e4b04e944d9d89.png",
"https://scitools.github.io/test-iris-imagehash/images/v4/ea05392995bac6d691ce3f21666569d86a96c6360ee195cb91e8ce54953b313b.png",
"https://scitools.github.io/test-iris-imagehash/images/v4/ea05392995bac6d691ea3f21666569d86a97c6320ee195cb91e8ce559539391b.png"
]

image

@bjlittle
Copy link
Member Author

bjlittle commented Apr 9, 2021

@jamesp I suggest we bank this as an issue on iris to investigate further - it's clearly a mpl-ism rather than iris. Perhaps a genuine regression, hence the big hamming distance (clearly I was "Accept"-tastic for the tests)

That would allow us to move forwards here. What do you think?

@jamesp
Copy link
Member

jamesp commented Apr 9, 2021

Yes agree, this doesn't seem a blocker to merging.

@bjlittle
Copy link
Member Author

bjlittle commented Apr 9, 2021

Thinking about this it would be nice to capture or integrate the output of idiff here so that the job of the review is much easier.

I'll have a think... 🤔

1 similar comment
@bjlittle
Copy link
Member Author

bjlittle commented Apr 9, 2021

Thinking about this it would be nice to capture or integrate the output of idiff here so that the job of the review is much easier.

I'll have a think... 🤔

@jamesp
Copy link
Member

jamesp commented Apr 9, 2021

@bjlittle @trexfeathers To be clear, I'm happy for this to be merged, but currently am not marked as a reviewer nor repo owner so cannot press the button

@bjlittle
Copy link
Member Author

bjlittle commented Apr 9, 2021

@jamesp I've added the SciTools/admins group with Admin rights on this repo (sorry, I thought that was already setup)

You should now be good to go 👍

@jamesp jamesp merged commit 47be6cb into SciTools:gh-pages Apr 9, 2021
@bjlittle
Copy link
Member Author

bjlittle commented Apr 9, 2021

@jamesp Awesome, thanks 🥳

@bjlittle bjlittle deleted the support-mpl-341 branch September 21, 2021 07:04
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.

3 participants