convert_units: raise exception when unit is unknown #2700

rcomer · 2017-08-01T10:11:06Z

Fix for #1977. If the units on a cube are unknown, the convert_units method should raise an exception. I consider the current behaviour of just setting the units to be a bug. If a user has not realised the metadata in the data file is incomplete, then the cube could end up with spurious data values.

A couple of things I'm not sure of:

I couldn't figure out if the class definitions in iris.exceptions were in any particular order, so just put the new one at the end.
Did I put the whatsnew in the right place?

rcomer · 2017-08-04T13:56:22Z

Travis is telling me it can't find Cartopy. I'm pretty sure my changes didn't cause this!

ajdawson · 2017-08-04T15:50:38Z

Travis is telling me it can't find Cartopy. I'm pretty sure my changes didn't cause this!

Internet pixies... they've been cleared out now.

Did I put the whatsnew in the right place?

This is really an incompatible change rather than a bugfix, although this may be a matter of opinion. I can imagine someone writing code that relies on this behaviour though.

rcomer · 2017-08-05T14:15:35Z

Thanks @ajdawson, I've now moved the whatsnew to the incompatible changes section.

DPeterK

This is a good start but I think there are a few bits that could be addressed to be improved.

DPeterK · 2017-08-07T09:30:44Z

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

+class Test_convert_units(tests.IrisTest):
+    def test_convert_unknown_units(self):
+        cube = iris.cube.Cube(1)
+        with self.assertRaises(UnitConversionError):


Please can you use the assertRaisesRegexp method here, as elsewhere in the Iris tests codebase. See https://github.com/SciTools/iris/blob/master/lib/iris/tests/unit/cube/test_Cube.py#L1309-L1314 for an example of this. The benefit is that we can check that not only the right error is being raised, but the right error in the right place (i.e. the error messages match).

DPeterK · 2017-08-07T11:38:15Z

docs/iris/src/whatsnew/2.0a0.rst

 * The :meth:`~iris.cube.Cube.lazy_data` method no longer accepts any arguments.

+* Attempting to use the :meth:`iris.cube.Cube.convert_units` method on a cube
+  where the existing unit is unknown will raise a :data:`UnitConversionError`


The situation (a cube has units unknown) is quite buried in this sentence structure. Perhaps we could rewrite this to:

"Using :meth:iris.cube.Cube.convert_units on a cube with unknown units will result in a :data:dataConversionError being raised."

DPeterK · 2017-08-07T12:17:09Z

lib/iris/cube.py

+        if self.units.is_unknown():
+            raise iris.exceptions.UnitConversionError(
+                'cannot convert from unknown units to {!s}, '
+                'set the "units" attribute instead'.format(unit))


I think the second line of the error message may confuse rather than help, so perhaps we can drop it. I'd also suggest printing the repr of the unit's name in the error message; that is:

"cannot convert from 'unknown' units to {!r}.".format(unit.origin)

I think the second line of the error message may confuse rather than help

I disagree, a hint to the correct way to do this is highly valuable. The wording could be changed though. What did you think might confuse @dkillick?

I guess it depends on why the error came up. If a user knew the unit was unknown and deliberately used the method to set it (as you suggested in your first comment @ajdawson) then telling them the right way to do it makes sense. If it came up because the data file was dodgy and the user didn't notice they weren't converting from anything (which has happened to me, and prompted posting the issue to begin with) probably all you need is to flag up that the cube unit is unknown.

What about something like 'Cannot convert from unknown units. The "cube.units" attribute may be set directly.' With no implication that what you are setting the unit to is the same as what you want to convert it to.

OK that is a fair point, the second line may be confusing under that circumstance. I can't think of a good catch-all alternative so I'm happy for you to remove as suggested by @dkillick, or if you want to try and re-word then please feel free.

I disagree

Great 👍 I had exactly the same thought process but went for suggesting the easy solution, which was just to remove that section. What I think might confuse is the current wording which I don't think makes clear quite what needs to be done. I'm also not sure how common it is for other error messages in Iris to give a recommendation on how to fix the problem, which was my other, uncommented concern here.

That said I can certainly see the benefit of giving a suggestion in this case. But to be clear that might have to look like the following...

("cannot convert from 'unknown' units to {unit!r}. " "Consider setting the cube's units instead, with `cube.units = cf_units.Unit({unit!r})`.".format(unit=unit.origin))

And maybe at this point we return to the second line confusing rather than helping...

What about something like 'Cannot convert from unknown units. The "cube.units" attribute may be set directly.'

@rcomer I like that - it is clear and descriptive without being complicated. If you could change the error message to that it would be great 👍

DPeterK · 2017-08-07T12:21:00Z

Hi @rcomer - thanks for this proposed change, which looks to improve some odd silent behaviour in Iris. You've also chosen a good time to propose this incompatible change just in time for Iris v2.0!

I've made some comments on some things that will need changing, which are all minor but I think necessary. I'd also like you to make the same set of changes again but for coord.convert_units - so the same logic change to this method, a test and a whatsnew entry. Then I think we'll be done!

rcomer · 2017-08-07T13:40:35Z

Thanks for the feedback @dkillick. I'm actually on leave for the next two weeks (browsing GitHub from my sofa, as you do). So, assuming there's no major hurry, I'll address it properly when I get back.

rcomer · 2017-08-21T12:47:47Z

OK, I think I have covered everything now. The Travis errors appear to all be timeouts. Do I need to do anything about the CLA? The Met Office already owns all my work...

ajdawson · 2017-08-21T17:36:23Z

Do I need to do anything about the CLA? The Met Office already owns all my work...

You don't need to sign the document but you do need to add your name to the contributors.json file in the SciTools/scitools.org.uk repository via PR.

rcomer · 2017-08-21T17:40:24Z

Thanks @ajdawson, I've had a notification that suggests @pelson has now added me to the contributors file in his branch.

ajdawson · 2017-08-21T18:09:31Z

docs/iris/src/whatsnew/2.0a0.rst


 * The :meth:`~iris.cube.Cube.lazy_data` method no longer accepts any arguments.

+* Using :meth:`~iris.cube.Cube.convert_units` on a cube with unknown units will


"cube or coordinate" instead of writing the whole sentence out twice?

I thought about that, but then you wouldn't have the links to the documentation for the methods. I have no strong opinion either way.

rcomer · 2017-08-25T13:45:52Z

I did a rebase to pick up #2757, but I think the internet pixies might be playing again:

urllib2.URLError: <urlopen error [Errno -2] Name or service not known>

The command "python -c 'import cartopy; cartopy.io.shapereader.natural_earth()'" failed and exited with 1 during .

pelson · 2017-08-25T15:38:15Z

lib/iris/tests/unit/coords/test_Coord.py

+                'The "coord.units" attribute may be set directly.')
+        with self.assertRaisesRegexp(UnitConversionError, emsg):
+            coord.convert_units('degrees')
+


Tests are probably going to freak out about the PEP8 of your newlines here...

I have run the pep8 command on my local copy of that file and got no output.

pelson · 2017-10-16T13:40:03Z

@dkillick - are you able to go through your review again and confirm the items that are outstanding please? I think there have been a few changes since your last comments, and they may now be addressed.

Thanks 😄

DPeterK · 2017-10-24T11:11:43Z

@rcomer thanks for pressing on with this – it's a very beneficial addition 👍

rcomer force-pushed the fix-unknown-convert branch from e17e216 to 35da692 Compare August 1, 2017 11:50

ajdawson added this to the v2.0 milestone Aug 4, 2017

DPeterK requested changes Aug 7, 2017

View reviewed changes

pelson added the Blocked: CLA needed See https://scitools.org.uk. Submit the form at: https://scitools.org.uk/cla/v4/form label Aug 21, 2017

rcomer changed the title ~~cube.convert_units: raise exception when unit is unknown~~ convert_units: raise exception when unit is unknown Aug 21, 2017

pelson added the Blocked: CLA needed See https://scitools.org.uk. Submit the form at: https://scitools.org.uk/cla/v4/form label Aug 21, 2017

ajdawson reviewed Aug 21, 2017

View reviewed changes

pelson removed the Blocked: CLA needed See https://scitools.org.uk. Submit the form at: https://scitools.org.uk/cla/v4/form label Aug 22, 2017

rcomer added 3 commits August 25, 2017 14:17

fix convert_units from unknown

1b9644b

shift whatsnew for cube.convert_unit

dfe8dfc

convert_units from unknown review changes

40c7e8a

rcomer force-pushed the fix-unknown-convert branch from d87c17e to 40c7e8a Compare August 25, 2017 13:19

pelson reviewed Aug 25, 2017

View reviewed changes

pelson approved these changes Aug 25, 2017

View reviewed changes

pelson assigned DPeterK Oct 24, 2017

DPeterK approved these changes Oct 24, 2017

View reviewed changes

DPeterK merged commit ce9d554 into SciTools:master Oct 24, 2017

DPeterK mentioned this pull request Oct 24, 2017

Move whatsnew entries to best location #2849

Merged

rcomer mentioned this pull request Oct 24, 2017

cube.convert_units converting from 'unknown' #1977

Closed

rcomer deleted the fix-unknown-convert branch December 7, 2018 16:49


		* The :meth:`~iris.cube.Cube.lazy_data` method no longer accepts any arguments.

		* Using :meth:`~iris.cube.Cube.convert_units` on a cube with unknown units will

convert_units: raise exception when unit is unknown #2700

convert_units: raise exception when unit is unknown #2700

Uh oh!

Conversation

rcomer commented Aug 1, 2017

Uh oh!

rcomer commented Aug 4, 2017

Uh oh!

ajdawson commented Aug 4, 2017

Uh oh!

rcomer commented Aug 5, 2017

Uh oh!

DPeterK left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DPeterK commented Aug 7, 2017

Uh oh!

rcomer commented Aug 7, 2017

Uh oh!

rcomer commented Aug 21, 2017

Uh oh!

ajdawson commented Aug 21, 2017

Uh oh!

rcomer commented Aug 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rcomer commented Aug 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pelson commented Oct 16, 2017

Uh oh!

DPeterK commented Oct 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rcomer commented Aug 21, 2017 •

edited

Loading