Repository navigation
Nemobug2 - bug fixes for a certain set of NEMO output - #138
Conversation
|
Have just resolved a trivial conflict (in the Changelog) to help me to review from a clean state. Now closing and re-opening the PR to trigger the new CI jobs which will help me to see the influence on the tests across Python versions and platforms. |
|
Going to build on top of the initial PR @bnlawrence, since for setting up the reference conda environment and the CI jobs to run the tests with and without image comparison, I have created a new directory of reference images specific to those expected to the reference environment, namely tests/v350-controlled-env-ref-images. So I will remove the updated |
|
Mid-way through updating the PR to account for the new reference environment and matched reference plots directory. Note the 'files change' count looks quite bloated because I am updating the images the docs point to here also since I'm adding a new example for the NEMO grid as part of work related to NEMO and the bug manifesting with it before this fix. |
|
(Tweaked the initial description to use a keyword which will auto-close the relevant Issue.) |
| fill, lines, blockfill, blockfill_fast = contour._resolve_curvilinear_render_defaults( | ||
| data=data, | ||
| kwargs={}, | ||
| fill=True, | ||
| lines=True, | ||
| blockfill=False, | ||
| blockfill_fast=None, | ||
| ) | ||
|
|
||
| assert fill is False | ||
| assert lines is False | ||
| assert blockfill is True | ||
| assert blockfill_fast is True |
There was a problem hiding this comment.
This is explicitly setting lines to True and then testing that lines is False - obviously the wrong test and one illustration of how AI-generated tests really need to be given a human check!
I noticed the incorrect default behaviour for showing or not the contour plot labelled lines, but this implies it might be set incorrectly in the logic. Will go in to fix either way before merging the PR.
There was a problem hiding this comment.
Hmm, in fact appears to be setting everything to a different value than what it tests it to be - I'll have to look into what's going on with this test.
There was a problem hiding this comment.
I now see what is happening and why it was implemented. I agree we can default to fast blockfill (hence blockfill = True, fill = False and blockfill_fast = True) for 2D lat-lon grids but I think we should maintain a lines = True default because otherwise it would make the default behaviour for line very confusing across different plots. To be honest I've wanted to change that default to false for a while because I think more times than not a user would not want contour lines shown, but I will change that default library-wide in a future update - not sure it should be bundled into this release.
Even more bloated now due to my rebuilding the docs here but it helps to get the whole codebase including docs in order in one fell swoop for the new NEMO fix and example. |
|
Edit: I realise this was a deliberate change for the 2D lat-lon default but in the case of The Default line behaviour with new NEMO data example plotDefaults to this, as expected, after the fix in a00dcf3, though I put 
|
|
Closing and re-opening to check CI jobs status. Happy with this now. |
|
The UKCP projection plots (examples 31 and 33) needed updating for the new default |
|
OK at this stage all of the tests without image comparison pass and the image comparison also passes except for the case of streamplots which look different in one small area on Mac than on Linux, hence Ubuntu CI job failing. That's a separate issue to this PR so we can ignore it here. Ready to merge now. |
This PR addresses the issue outlined in #126, by making some fixes for curvilinear coordinate for normal plotting and with blockfill. This PR replaces #137 which was on top of a previous PR and unnecessarily complicated.
(Original 3 commits supplemented by ~15 commits added by @sadielbartholomew to quickly address own review feedback and integrate with recent testing changes.)