Fix name loader problem #4933

pp-mo · 2022-08-25T17:52:20Z

Addresses #3288

~~Draft pending :~~

~~[x] tests for enhanced API~~
~~[x] whatsnew~~

pp-mo · 2022-09-07T17:22:30Z

I think this is okay to review now.

A couple of notes though ...
(1.) I ended up doing two different things here :
* (a) fix the name loaders
* (b) enforce rules on the expected context of cube.cell_methods.
I basically think that's ok, as it's all quite simple.
I have added two separate whatsnew entries, accordingly

(2.) I considered adding a test-file replicating the #3288 issue, and an integration test for the fix.
But ... the file provided there may not be for public consumption, the discussion there has gone very cold, and I'm not confident to produce correctly-formed test data myself.
So I think it's really not essential, and much simpler to just not bother with this

pp-mo · 2022-09-08T09:02:31Z

@volcan01010 would you be available to review ?

lbdreyer

This looks good to me, I had one small comment though.

It is a bit of a shame there is no extra testing for the name loader so it would be nice to get hold of some test data. Is there a way we could generate some?

lib/iris/fileformats/name_loaders.py

volcan01010 · 2022-09-12T14:34:27Z

Yes, I can have a look at this this week. It will be quite superficial, though, as I haven't worked with these data for a while.

pp-mo · 2022-09-12T14:39:57Z

This looks good to me, I had one small comment though.

It is a bit of a shame there is no extra testing for the name loader so it would be nice to get hold of some test data. Is there a way we could generate some?

As I explained above, we could ask @volcan01010 if he is prepared to release his sample file ??
None of the existing test files seem suitable.
I did test it on a modified version of one of the existing test files, but the problem is I can only guess as to what is correct within the format.

DPeterK

Just one comment from me on handling objects that aren't cell methods that show up in the list of cell methods. To be clear, I don't think this is something that needs to be fixed, but we could consider an alternative approach to erroring fast in the middle of a dataset load.

lib/iris/cube.py

pp-mo · 2022-09-12T16:20:18Z

None of the existing test files seem suitable. I did test it on a modified version of one of the existing test files, but the problem is I can only guess as to what is correct within the format.

Well, on a closer look I think we actually did have a suitable testfile all along, which was previously unused.
So I used that -- can someone please confirm that it covers the case ?

Meanwhile, apologies for my moving this + some other tests into slightly a more logical hierarchy, it just seemed time to get that done.

UPDATE: doubtful - read on!!

pp-mo · 2022-09-12T16:41:32Z

OK, I confused myself there.
There is no such pre-existing file in iris-test-data, only in my version !

I will fix this,
and perhaps now revert all the file moves as it makes the changed-files quite unreasonably messy ...

this goes in draft, while I fix it

pp-mo · 2022-09-12T22:57:48Z

This should pass, once SciTools/iris-test-data#82 is merged
(unless some other update is merged there first)

vsherratt

One minor comment about the naming here, but more importantly:

Having mentioned both NAME II and NAME III formats, they are both widely used in practice (due to a habit of copying NAME config without necessarily reviewing options like the output format), so I would suggest testing both. I can find/create some example output if needed.
Can this PR please also be added as patch releases to both 3.2 and 3.3? The refactoring of the cube string representation code in 3.2 combines with this bug to mean we often cannot print a cube. While I believe we could do almost everything else with it (except save as originally raised in the linked issue), the lack of printing does not give anyone the confidence to try, so we are essentially stuck using older versions of iris for NAME.

lib/iris/tests/test_name.py

trexfeathers · 2022-09-21T09:28:00Z

Can this PR please also be added as patch releases to both 3.2 and 3.3? The refactoring of the cube string representation code in 3.2 combines with this bug to mean we often cannot print a cube

Offline discussion of the specifics: a v3.3.1 bugfix would serve, so that's what we will aim for.

volcan01010 · 2022-09-22T11:00:56Z

I've had a look through the PR and it looks good to me. I haven't worked on that Iris project for a while, so unfortunately I'm not up to speed to do a more detailed review. Please feel free to incorporate the test data file into the project.

pp-mo · 2022-09-22T15:01:07Z

Many thanks @bsherratt
Working slightly in the dark here, as don't actually know much about NAME, so help is welcome !

Can this PR please also be added as patch releases to both 3.2 and 3.3? The refactoring of the cube string representation code in 3.2 combines with this bug to mean we often cannot print a cube. While I believe we could do almost everything else with it (except save as originally raised in the linked issue), the lack of printing does not give anyone the confidence to try, so we are essentially stuck using older versions of iris for NAME.

From internal discussion (peloton meeting), it seems this really is needed, so yes, we will commit to a 3.3.1 release.
(See #4976 for the beginnings)

Actually, the example file is NAME II format, not NAME III.

Ok I will rename the new file + fix it here and in SciTools/iris-test-data#82

Having mentioned both NAME II and NAME III formats, they are both widely used in practice (due to a habit of copying NAME config without necessarily reviewing options like the output format), so I would suggest testing both. I can find/create some example output if needed.

This sounds a useful improvement, but we should handle it as a separate issue ?
#4979
I will contact you offline about getting some suitable testfiles ...

pp-mo · 2022-09-23T09:43:23Z

Yay, GTG !
with thanks, Merges invited ? ...

trexfeathers

Thanks @pp-mo! Some tiny testing changes for you, also:

Don't forget to re-target to `v3.3.x`

lib/iris/tests/test_name.py

lib/iris/tests/unit/cube/test_Cube.py

lib/iris/cube.py

docs/src/whatsnew/latest.rst

pp-mo · 2022-09-23T14:15:49Z

self.assertEqual and self.assertRaisesRegex can be converted to pytest style

Ok I have done that now + I think it looks pretty neat. f6853de

However, It may be appropriate for @lbdreyer to also check out + OK my use of the fixture here ...

So... I have defined a fixture as a class-instance-method, and labelled it "autouse=True", and it assigns data to "self" properties.
Which is really close to the unittest "setUp" approach -- so I just hope that this is appropriate !!

It certainly seems to work, and I think it makes sense : on debugging, I find that the 'self' instance of the "Test__cell_methods" class which the fixture receives is unique for each test method invocation. So I assume that this 'self' therefore "is the test instance", and can be used to hold per-instance data as in the older style.
However, I'd just like a bit more confirmation that this makes sense to someone else, as I don't see this suggestion in the Pytest docs and basically got here more by trial-and-error than from the docs !!

pp-mo · 2022-09-23T14:17:30Z

Meanwhile ...

Don't forget to re-target to v3.3.x

Good spot.
However, I think it's best to let #4976 go first, as that establishes the whatsnew section for 3.3.1.
Rather than changing this + making a conflict, let's wait for that, after which I will fix/rebase this one to build on that accordingly.

….16.

pp-mo · 2022-09-26T13:44:52Z

Thanks @trexfeathers
I have now re-targetted and fixed, and I think will now be OK.
That includes fixing the outstanding problem with the post-3.3 whatsnew : added an indent to put it inside the v.3.3.1 dropdown.

But I've had to squash commits to sort out the changes 😞

There was also some suggestion we might want to add the netcdf version pin from #4968 onto 3.3.x, as otherwise we will probably get occasional docs build failures.
But I'm hoping we can address that separately.

trexfeathers

So GitHub is telling me you've changed pretty much everything since my review on Friday, but I can only see the changes I was expecting so I'm assuming that's just the force-pushing messing things up.

lbdreyer · 2022-09-26T14:22:00Z

So the docs build is failing again (see https://readthedocs.org/projects/scitools-iris/builds/ for the latests builds) I restarted the job a couple of times but it's getting the same HDF error. Interestingly when @pp-mo re ran the build it worked the second time

trexfeathers · 2022-09-26T14:23:17Z

So the docs build is failing again (see https://readthedocs.org/projects/scitools-iris/builds/ for the latests builds) I restarted the job a couple of times but it's getting the same HDF error. Interestingly when @pp-mo re ran the build it worked the second time

There was also some suggestion we might want to add the netcdf version pin from #4968 onto 3.3.x, as otherwise we will probably get occasional docs build failures.
But I'm hoping we can address that separately.

pp-mo · 2022-09-27T08:44:51Z

Belated thanks @trexfeathers
This was one of those occasions when I couldn't get a straight rebase to work for me.
It is unavoidably tough when a rebase/squash interrupts the comment/response cycle, but I needed it.
Thanks for wading through !
⭐ 🥇

trexfeathers · 2022-09-27T08:48:08Z

This was one of those occasions when I couldn't get a straight rebase to work for me.

@pp-mo next time I'm reviewing feel free to merge in the updates instead of rebasing. It's better for my reviewing workflow and would be easier on you for resolving conflicts.

(And obvs since this is a PR: squashing saves us from any long term nastiness with history)

* v3.3.x: Update whatsnew for 3.3.1 release (SciTools#5002) Port dependency fixes to `v3.3.x`. (SciTools#4992) Fix name loader problem (SciTools#4933) Reinstate CubeList._repr_html_() (SciTools#4976) # Conflicts: # .github/workflows/ci-tests.yml # requirements/ci/nox.lock/py310-linux-64.lock # requirements/ci/nox.lock/py38-linux-64.lock # requirements/ci/nox.lock/py39-linux-64.lock # requirements/ci/py310.yml # requirements/ci/py38.yml # requirements/ci/py39.yml # setup.cfg

stephenworsley assigned pp-mo Aug 31, 2022

pp-mo force-pushed the valid_cell_methods branch from 01f6b97 to 3533ce1 Compare September 7, 2022 16:53

pp-mo marked this pull request as ready for review September 7, 2022 17:14

pp-mo mentioned this pull request Sep 7, 2022

iris.save crashes for NetCDF for cube with no cell_methods #3288

Closed

lbdreyer reviewed Sep 12, 2022

View reviewed changes

lib/iris/fileformats/name_loaders.py Show resolved Hide resolved

DPeterK reviewed Sep 12, 2022

View reviewed changes

lib/iris/cube.py Show resolved Hide resolved

pp-mo force-pushed the valid_cell_methods branch from 3533ce1 to 3922d79 Compare September 12, 2022 16:16

pp-mo marked this pull request as draft September 12, 2022 16:41

pp-mo force-pushed the valid_cell_methods branch from 3922d79 to 6c043c7 Compare September 12, 2022 22:45

pp-mo mentioned this pull request Sep 12, 2022

NAME testfile with no time averaging. SciTools/iris-test-data#82

Merged

pp-mo marked this pull request as ready for review September 12, 2022 22:58

bjlittle self-requested a review September 14, 2022 09:13

vsherratt reviewed Sep 20, 2022

View reviewed changes

lib/iris/tests/test_name.py Outdated Show resolved Hide resolved

pp-mo mentioned this pull request Sep 22, 2022

Test more types of NAME file #4979

Closed

trexfeathers assigned trexfeathers and unassigned pp-mo Sep 23, 2022

trexfeathers self-requested a review September 23, 2022 11:12

trexfeathers requested changes Sep 23, 2022

View reviewed changes

lib/iris/tests/test_name.py Outdated Show resolved Hide resolved

lib/iris/tests/unit/cube/test_Cube.py Outdated Show resolved Hide resolved

lib/iris/cube.py Show resolved Hide resolved

docs/src/whatsnew/latest.rst Outdated Show resolved Hide resolved

pp-mo mentioned this pull request Sep 26, 2022

Reinstate CubeList._repr_html_() #4976

Merged

1 task

pp-mo force-pushed the valid_cell_methods branch from f6853de to 544bd1d Compare September 26, 2022 12:53

pp-mo changed the base branch from main to v3.3.x September 26, 2022 12:54

pp-mo force-pushed the valid_cell_methods branch 2 times, most recently from e765f71 to efa3482 Compare September 26, 2022 13:03

Fix name loader problem; enforce expected type of all cube cell_methods.

9064372

pp-mo force-pushed the valid_cell_methods branch from efa3482 to 9064372 Compare September 26, 2022 13:12

pp-mo added 2 commits September 26, 2022 14:15

Review change: fix import order.

7f9c45c

Update imagerepo.json for skipped changes from test-iris-imagehash v0…

9b00127

….16.

trexfeathers approved these changes Sep 26, 2022

View reviewed changes

trexfeathers merged commit d38c504 into SciTools:v3.3.x Sep 26, 2022

pp-mo deleted the valid_cell_methods branch September 26, 2022 15:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix name loader problem #4933

Fix name loader problem #4933

pp-mo commented Aug 25, 2022 •

edited

Loading

pp-mo commented Sep 7, 2022 •

edited

Loading

pp-mo commented Sep 8, 2022

lbdreyer left a comment

volcan01010 commented Sep 12, 2022

pp-mo commented Sep 12, 2022

DPeterK left a comment

pp-mo commented Sep 12, 2022 •

edited

Loading

pp-mo commented Sep 12, 2022 •

edited

Loading

pp-mo commented Sep 12, 2022 •

edited

Loading

vsherratt left a comment

trexfeathers commented Sep 21, 2022

volcan01010 commented Sep 22, 2022

pp-mo commented Sep 22, 2022

pp-mo commented Sep 23, 2022

trexfeathers left a comment

pp-mo commented Sep 23, 2022 •

edited

Loading

pp-mo commented Sep 23, 2022

pp-mo commented Sep 26, 2022

trexfeathers left a comment

lbdreyer commented Sep 26, 2022

trexfeathers commented Sep 26, 2022

pp-mo commented Sep 27, 2022 •

edited

Loading

trexfeathers commented Sep 27, 2022 •

edited

Loading

Fix name loader problem #4933

Fix name loader problem #4933

Conversation

pp-mo commented Aug 25, 2022 • edited Loading

pp-mo commented Sep 7, 2022 • edited Loading

pp-mo commented Sep 8, 2022

lbdreyer left a comment

Choose a reason for hiding this comment

volcan01010 commented Sep 12, 2022

pp-mo commented Sep 12, 2022

DPeterK left a comment

Choose a reason for hiding this comment

pp-mo commented Sep 12, 2022 • edited Loading

pp-mo commented Sep 12, 2022 • edited Loading

pp-mo commented Sep 12, 2022 • edited Loading

vsherratt left a comment

Choose a reason for hiding this comment

trexfeathers commented Sep 21, 2022

volcan01010 commented Sep 22, 2022

pp-mo commented Sep 22, 2022

pp-mo commented Sep 23, 2022

trexfeathers left a comment

Choose a reason for hiding this comment

Don't forget to re-target to v3.3.x

pp-mo commented Sep 23, 2022 • edited Loading

pp-mo commented Sep 23, 2022

pp-mo commented Sep 26, 2022

trexfeathers left a comment

Choose a reason for hiding this comment

lbdreyer commented Sep 26, 2022

trexfeathers commented Sep 26, 2022

pp-mo commented Sep 27, 2022 • edited Loading

trexfeathers commented Sep 27, 2022 • edited Loading

pp-mo commented Aug 25, 2022 •

edited

Loading

pp-mo commented Sep 7, 2022 •

edited

Loading

pp-mo commented Sep 12, 2022 •

edited

Loading

pp-mo commented Sep 12, 2022 •

edited

Loading

pp-mo commented Sep 12, 2022 •

edited

Loading

Don't forget to re-target to `v3.3.x`

pp-mo commented Sep 23, 2022 •

edited

Loading

pp-mo commented Sep 27, 2022 •

edited

Loading

trexfeathers commented Sep 27, 2022 •

edited

Loading