Skip to content

Fix problem with factories in iris.util.new_axis().#3263

Merged
lbdreyer merged 5 commits intoSciTools:v2.2.xfrom
pp-mo:newaxis_factories
Feb 27, 2019
Merged

Fix problem with factories in iris.util.new_axis().#3263
lbdreyer merged 5 commits intoSciTools:v2.2.xfrom
pp-mo:newaxis_factories

Conversation

@pp-mo
Copy link
Member

@pp-mo pp-mo commented Feb 7, 2019

It was found that adding an axis to a cube with an aux-factory had a subtle bug, which then causes errors when you copy the result :

Example:

>>> cube
<iris 'Cube' of mass_fraction_of_ozone_in_air / (kg kg-1) (model_level_number: 63; grid_latitude: 182; grid_longitude: 146)>
>>> 
>>> # Presumably, something here is wrong ...
... cube2 = iutil.new_axis(cube, 'time')
>>> cube2
<iris 'Cube' of mass_fraction_of_ozone_in_air / (kg kg-1) (time: 1; model_level_number: 63; grid_latitude: 182; grid_longitude: 146)>
>>> 
>>> # ... but this is what actually fails :
... cube3 = cube2.copy()
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
  File "/opt/scitools/environments/experimental/2019_01_31-1/lib/python3.6/site-packages/iris/cube.py", line 2967, in copy
    cube = self._deepcopy(memo, data=data)
  File "/opt/scitools/environments/experimental/2019_01_31-1/lib/python3.6/site-packages/iris/cube.py", line 3003, in _deepcopy
    new_cube.add_aux_factory(factory.updated(coord_mapping))
  File "/opt/scitools/environments/experimental/2019_01_31-1/lib/python3.6/site-packages/iris/aux_factory.py", line 156, in updated
    coord = new_coord_mapping[id(coord)]
KeyError: 140318647616680
>>> 

The new addition to the test code should demonstrate this (fails when run against old code).

@pp-mo
Copy link
Member Author

pp-mo commented Feb 7, 2019

What was actually wrong is :

  • the dependencies within a factory must be the actual coords in the cube for cube.copy() to work..
  • .. because otherwise the factory dependencies don't show up in the old-new 'coord_mapping' that the copy operation constructs
  • .. whereas, "new_axis" returns a cube whose factory has its own independent copies of the original coords : so that won't copy properly

self._assert_cube_notis(res, cube)

# Check that factory dependencies are actual coords within the cube.
# Addresses a former bug : see XXXX
Copy link
Member

Choose a reason for hiding this comment

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

I assume you'll update this with the issue number?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok

Copy link
Member

@lbdreyer lbdreyer left a comment

Choose a reason for hiding this comment

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

This looks good 👍 ! Thanks @pp-mo!

Just need to update the comment with the correct issue/Pr number

@pp-mo
Copy link
Member Author

pp-mo commented Feb 19, 2019

Thanks @lbdreyer !
I will fix that shortly.
But I want to just hold off a little on this one, as I have since been made aware of a possibly-related problem in some user code, which seems to have a similar error ...

@pp-mo
Copy link
Member Author

pp-mo commented Feb 21, 2019

problem in some user code, which seems to have a similar error

That was not related after all.

Let's go ahead with this !
-- Except, I will retarget against v2.2.x
now re-targetted against v2.2.x

@pp-mo pp-mo changed the base branch from master to v2.2.x February 21, 2019 17:31
@SciTools-assistant SciTools-assistant added the Blocked: CLA needed See https://scitools.org.uk. Submit the form at: https://scitools.org.uk/cla/v4/form label Feb 21, 2019
@pp-mo pp-mo force-pushed the newaxis_factories branch from e0aa55a to 0ceb0b6 Compare February 21, 2019 17:36
@SciTools-assistant SciTools-assistant removed the Blocked: CLA needed See https://scitools.org.uk. Submit the form at: https://scitools.org.uk/cla/v4/form label Feb 21, 2019
@lbdreyer lbdreyer added this to the v2.2.1 milestone Feb 22, 2019
@pp-mo
Copy link
Member Author

pp-mo commented Feb 27, 2019

Following offline discussion with @lbdreyer :
Bugfix release whatsnew contributions should go in a separate block in the minor-version whatsnew documentation.
So here, moved note from a contributions file into a 'Bugs fixed in v2.2.1' section in the existing v2.2 whatsnew.
This parallels what we see in, for example, the v1.7 whatsnew bugfixes

@lbdreyer
Copy link
Member

lbdreyer commented Feb 27, 2019

Great, thanks @pp-mo !

@lbdreyer lbdreyer merged commit 864d5e8 into SciTools:v2.2.x Feb 27, 2019
lbdreyer pushed a commit to lbdreyer/iris that referenced this pull request May 28, 2019
* Fix problem with factories from iris.util.new_axis().

* Added whatsnew.

* Licence header fix.

* Reference PR in comment.

* Revised whatsnew.
lbdreyer pushed a commit to lbdreyer/iris that referenced this pull request Jun 3, 2019
* Fix problem with factories from iris.util.new_axis().

* Added whatsnew.

* Licence header fix.

* Reference PR in comment.

* Revised whatsnew.
lbdreyer pushed a commit to lbdreyer/iris that referenced this pull request Jun 3, 2019
* Fix problem with factories from iris.util.new_axis().

* Added whatsnew.

* Licence header fix.

* Reference PR in comment.

* Revised whatsnew.
@pp-mo pp-mo deleted the newaxis_factories branch March 18, 2022 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants