Hybrid pressure #46
Hybrid pressure #46
Conversation
| @@ -473,7 +473,9 @@ def add_dim_coord(self, dim_coord, data_dim=None): | |||
|
|
|||
| # Check compatibility with the shape of the data | |||
| if dim_coord.shape[0] != self.shape[data_dim]: | |||
| raise ValueError('The length of %r does not match the length of cube dimension %d' % (dim_coord, data_dim)) | |||
| msg = 'Incompatible lengths: cube dimension {} => {}; {} => {}.' | |||
| raise ValueError(msg.format(data_dim, self.shape[data_dim], | |||
There was a problem hiding this comment.
We can more be more explicit and say that the lengths are unequal rather than incompatible. Saying they are incompatible may not help the user understand the problem (i.e. they are not equal).
There was a problem hiding this comment.
Exception message on line 412 (in Cube.add_aux_coord()) should match this message.
|
As an aside, orography (and surface pressure) are coordinates so they cannot (currently) have missing data. It seems reasonable that they might, so would any changes be needed to handle this appropriately. My gut tells me careful scattering of |
| if len(reference_cubes[arg.name]) > 1: | ||
| all = iris.cube.CubeList(reference_cubes[arg.name]) | ||
| reference_cubes[arg.name] = all.merge() | ||
| src = reference_cubes[arg.name][0] |
There was a problem hiding this comment.
Why do you take the first cube in the list rather than the last (as before)? If the merge can't find anything to merge and does nothing, you are changing the behaviour.
There was a problem hiding this comment.
Good point. I'd been assuming the merge would be successful. I'll add a warning if the merge wasn't completely successful (i.e. it resulted in more than one cube), and make sure I always pick the last one.
|
This PR is associated with #1. |
|
The missing-data-in-coordinates issue should be taken care of by the higher precedence of masked arrays. ie. |
|
Rebased - ready for review. |
| @@ -1008,9 +1011,12 @@ def is_monotonic(self): | |||
| class AuxCoord(Coord): | |||
| """A CF auxiliary coordinate.""" | |||
| @staticmethod | |||
| def from_coord(coord): | |||
| def from_coord(coord, points=None): | |||
There was a problem hiding this comment.
I don't like this. It'll blow up if the points are incompatible with the bounds of the source coord. I know you could add a further bounds arg and implement the same logic of wiping bounds if only points are provided, but I'm not keen on adding arguments (or this logic) as it'll encourage use of this somewhat questionable method (I know I created it the first place!). If you just want the coord's metadata (which is the case if you are providing alternative points and bounds) then I'd prefer something like:
new_coord = AuxCoord(new_points)
new_coord.metadata = old_coord.metadataThere was a problem hiding this comment.
Agreed - especially as it's only used in one place (interpolation).
|
I think this is a side-effect of an artificial separation of AuxCoord from Coord. |
| factory=factory_name)) | ||
| warnings.warn(msg.format(filenames=filenames, | ||
| factory=factory_name)) | ||
| break |
There was a problem hiding this comment.
Why do you break here? A continue (or equivalently nothing at all) seems like a better idea as you might be able to make other factories of the cube.
There was a problem hiding this comment.
Agreed. The break is a hangover from the old in-line code.
|
Nearly there. @rhattersley - should we squash it all into a single commit for merging to keep the history on master a little cleaner? |
|
Given the convoluted mess I think a bit of a squash is in order. |
Merge time-varying reference cubes, and upgrade interpolation to cope. Units checking in hybrid coordinate constructors.
* header dates corrected whitespace removed
* header dates corrected whitespace removed
* header dates corrected whitespace removed
Key features: