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

Cubeprinter adopt #4206

Merged
merged 23 commits into from
Aug 10, 2021
Merged

Cubeprinter adopt #4206

merged 23 commits into from
Aug 10, 2021

Conversation

pp-mo
Copy link
Member

@pp-mo pp-mo commented Jun 23, 2021

🚀 Pull Request

Description

Followon from #3998
Final small formatting changes
Unsurprisingly, I had to squash + rebase from #3998 to get this testing.

Closes : #4211

Consult Iris pull request check list

@pp-mo pp-mo marked this pull request as draft June 23, 2021 11:21
@pp-mo
Copy link
Member Author

pp-mo commented Jun 23, 2021

Surprisingly, this caused no obvious problems.
So now I'm now going to crowbar-replace the old with the new styles, and see what happens ...

@pp-mo
Copy link
Member Author

pp-mo commented Jun 23, 2021

22 failures is really not that bad.
I think, at that rate, that I will simply adjust the reference files.
-- whereas I was considering a "clever" testing strategy more akin to the 'tolerant comparison' techniques used above.

From discussion with @bjlittle I don't think we need that -- simpler to just change the references.
Likewise, decided to remove the additional colons in scalar-coord and attribute outputs, as again it doesn't need to be that close to the existing results.
So let's have colons in the section headings, but not elsewhere.

@pp-mo
Copy link
Member Author

pp-mo commented Jun 24, 2021

I don't understand the latest docstest fail.
When I run the doctests locally, it insists that the result of printing a cube.metadata._asdict() shows as an OrderedDict.
And I can confirm that it looks like that, e.g.

>>> print(Cube([0]).metadata._asdict())
OrderedDict([('standard_name', None), ('long_name', None), ('var_name', None), ('units', Unit('unknown')), ('attributes', {}), ('cell_methods', ())])
>>> 

For me it's doing that in my test env too, and with older versions of Iris.

But in the CI testing it prints out as an ordinary dict.
And that is in spite of my running the tests via nox : nox -s doctest-3.7
-- which surely ought to take care of any environment differences ??
Possibly it is a lockfiles thing ?

UPDATE:
It is a Python 37/38 difference :

$ conda activate py37
$ python
Python 3.7.10 | packaged by conda-forge | (default, Feb 19 2021, 16:07:37) 
[GCC 9.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from collections import namedtupl 
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: cannot import name 'namedtupl' from 'collections' (/tmp/persistent/miniconda3/envs/py37/lib/python3.7/collections/__init__.py)
>>> from collections import namedtuple
>>> nt = namedtuple('nt', ['a','b'])
>>> print(nt(a=1, b=2)._asdict())
OrderedDict([('a', 1), ('b', 2)])
>>> 
$ 
$ conda activate py38
$ python
Python 3.8.10 | packaged by conda-forge | (default, May 11 2021, 07:01:05) 
[GCC 9.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from collections import namedtuple
>>> nt = namedtuple('nt', ['a','b'])
>>> print(nt(a=1, b=2)._asdict())
{'a': 1, 'b': 2}
>>> 
$ 

So, I needed to run nox -s doctest-3.8 and not 3.7.
Which is what cirrus does.

@pp-mo pp-mo requested a review from bjlittle June 24, 2021 10:31
@pp-mo
Copy link
Member Author

pp-mo commented Jun 24, 2021

Yay, done at last.
@bjlittle very grateful for your eyes on this ..

@trexfeathers
Copy link
Contributor

Followon from #3998

Does this supercede #3998?

@pp-mo
Copy link
Member Author

pp-mo commented Jun 29, 2021

Does this supercede #3998?

Absolutely, yes.
( Sorry, sure I posted this earlier but it vanished? perhaps failed to hit the button )

@pp-mo
Copy link
Member Author

pp-mo commented Jun 29, 2021

Thanks for looking @bjlittle 👀
Just now spotted conflicts + fixing that ...

@pp-mo pp-mo force-pushed the cubeprinter_adopt branch 2 times, most recently from 1832cf7 to c13dfcc Compare June 29, 2021 15:09
@pp-mo
Copy link
Member Author

pp-mo commented Jun 29, 2021

Hit that spurious, intermittent failure of the animation test again.
Now fixed.

Unfortunately I re-spun it and so have lost the reference.
But there is a problem lurking there somewhere...
I think it is nothing to do with this PR, but also it did not go away with rebasing, so I guess we'll see it again sometime (on master)

@pp-mo
Copy link
Member Author

pp-mo commented Jul 22, 2021

Merged in the re-ordering from #4251 that ensures attributes come last, after cell-methods.
Fixed resulting test changes.
All passing now.
@bjlittle ready when you are !

@pp-mo pp-mo added this to the v3.1.0 milestone Jul 29, 2021
Copy link
Member

@bjlittle bjlittle left a comment

Choose a reason for hiding this comment

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

@pp-mo Great stuff 👍

Thanks for diligently wading through this unpleasant mire of Cube.summary refactorisation, the result is a vast improvement IMHO.

Just a couple of minor comments to service, then we can bank it 🍻

docs/src/further_topics/metadata.rst Outdated Show resolved Hide resolved
lib/iris/_representation/cube_printout.py Outdated Show resolved Hide resolved
lib/iris/_representation/cube_printout.py Outdated Show resolved Hide resolved
lib/iris/_representation/cube_printout.py Outdated Show resolved Hide resolved
lib/iris/_representation/cube_printout.py Outdated Show resolved Hide resolved
lib/iris/_representation/cube_printout.py Outdated Show resolved Hide resolved
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.

Review/revise ordering of cube-summary sections
3 participants