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

Undo changes from #40 (anomaly log plot) as these were not the appropriate fix. #44

Merged
merged 1 commit into from
May 4, 2021

Conversation

trexfeathers
Copy link
Contributor

Undo changes from #40 (anomaly log plot) as these were not the appropriate fix. The appropriate fix has been achieved via SciTools/iris#4115.

@trexfeathers trexfeathers requested review from jamesp and rcomer May 4, 2021 16:36
Copy link
Member

@rcomer rcomer left a comment

Choose a reason for hiding this comment

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

LGTM

@rcomer rcomer merged commit 3d37f12 into SciTools:gh-pages May 4, 2021
@trexfeathers trexfeathers deleted the undo_40 branch May 5, 2021 10:00
@pp-mo
Copy link
Member

pp-mo commented Jul 6, 2021

Could we have avoided removing this ?
Ideally, we should never remove things from this repo,
because it's not just iris::main that may be relying on it...

Removing this file has messed up the test in this PR
Because the PR is based against a different branch (mesh-data-model), the PR can't merge the change to imagerepo.json that removes the reference to this file -- because that is only in the main branch.
But, if I make that change on the PR branch, it will (eventually) cause a merge conflict when we try to merge the mesh-data-model branch back...

@trexfeathers
Copy link
Contributor Author

Could we have avoided removing this ?

I hear what you're saying, but so long as the image was in this repo it was possible for tests to pass despite invalid behaviour. Preserving invalid software behaviour to ease source control seems like a big anti-pattern to me. Would there have been a third way that I haven't thought of?

@pp-mo
Copy link
Member

pp-mo commented Jul 7, 2021

Could we have avoided removing this ?
I hear what you're saying, but so long as the image was in this repo it was possible for tests to pass despite invalid behaviour.

I still think this was wrong, really.
I found it causing more trouble

@pp-mo you also need to bring in the correct use of linscale from #4115.

But..

... Would there have been a third way that I haven't thought of?

Unfortunately no, I don't think there is.


The real problem here, possibly, is that we don't version our use of test-iris-imagehash at all -- we don't specify a commits, tag, or version of the repo.
So, all testing occurs against the current latest content.
So in that case, the big problem with any non-additive change is that it may break testing of older versions of Iris.
I think it should really be possible to go back to any older version of Iris (a release, or a trunk commit) and still have it pass all the tests (for comparison, obviously).

In this case, that included the problem PR, because being based on a feature-branch, subsequent changes to 'main' are not merged to it. So, it can only run against the latest test-iris-imagehash if it also contains the required changes to the test.

So, in hopes of coming up with some guiding principles.
All totally In my view ...

  • the result that was removed was "a valid answer to the wrong question".
  • but changing the question doesn't mean we should remove results that were "previously judged correct"

Of course, the mechanism does leave the door open to a future error producing the same image result, which would then be definitely a wrong answer, yet (erroneously) pass the test.
But I think we just have to trust that the "space of wrong results" is large enough that, statistically, cases like that will be very rare.
Whereas, deciding that what used to be "correct" no longer is, does risk breaking the workflow.
I think a viable alternative would be to version/tag the state of test-iris-imagehash, as we do with iris-test-data.

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