Fix name loader problem

[pp-mo]

Addresses #3288

~Draft pending :~

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

[pp-mo]

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]

@volcan01010 would you be available to review ?

[volcan01010]

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]

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.

[pp-mo]

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]

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]

This should pass, once https://github.com/SciTools/iris-test-data/pull/82 is merged (unless some other update is merged there first)

[trexfeathers]

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]

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]

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 https://github.com/SciTools/iris-test-data/pull/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 ? https://github.com/SciTools/iris/issues/4979 I will contact you offline about getting some suitable testfiles ...

[pp-mo]

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

[pp-mo]

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

Ok I have done that now + I think it looks pretty neat. https://github.com/SciTools/iris/pull/4933/commits/f6853deb1fa2695975fa049f24b3d40f92ff511e

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]

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.

[pp-mo]

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 :disappointed:

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

[lbdreyer]

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]

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 https://github.com/SciTools/iris/pull/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]

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]

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)