Switch to using DimCoord.from_regular#642
Conversation
There was a problem hiding this comment.
Do you know of any smart way of keeping the attribute definitions in sync? I'm concerned that a new attribute will be added in init which is expected, but is not added by this method.
There was a problem hiding this comment.
If we were calling the constructor, as I queried above, and this new attribute is expected, it'll be an arg and would fail if omitted?
There was a problem hiding this comment.
Do you know of any smart way of keeping the attribute definitions in sync?
In short ... no. 😒
There was a problem hiding this comment.
A method such as Coord._set_metadata doesn't help unless it's also used by Coord.__init__. But then how do we provide docstrings for the instance attributes?
There was a problem hiding this comment.
hmm...
i suppose, because init needs to accept them all and pass them to set_metadata, it's not hard to put them in init, as =None, with docstrings.
i'm not going to push for this but it would help protect these functions from drift.
There was a problem hiding this comment.
I'll see what the negative performance impact is of setting the values twice...
There was a problem hiding this comment.
Adding Coord._set_metadata completely negates the performance benefit. 😑
Plus there's still the question of how to handle attributes which are specific to DimCoord (i.e. currently just circular). One can't just override _set_metadata because it's used by Coord.__init__ which has no knowledge of the extra attributes. Anyway ... it's all hypothetical ...there's no point in the PR if there's no benefit 😉
|
Nice solution. 👍 conceptually, just interested in hearing your thoughts on the subject of ensuring that the classmethod generates an appropriately defined DimCoord object? |
There was a problem hiding this comment.
Is there a reason for calling object.__new__(cls) rather than cls.__new__(cls) or DimCoord.__new__(cls)?
There was a problem hiding this comment.
...or even just DimCoord(...) at the bottom?
There was a problem hiding this comment.
Presumably the point is to avoid a call to init() with its checks for monotonicity etc.
There was a problem hiding this comment.
would probably have to be cls(...)
There was a problem hiding this comment.
Is there a reason for calling
object.__new__(cls)rather thancls.__new__(cls)orDimCoord.__new__(cls)?
There's no particular reason for using object.__new__(cls) except object is (currently) the only class in the MRO which defines __new__. That said, it would be better as either Coord.__new__(cls) or super(DimCoord, cls).__new__() to allow for the subsequent addition of Coord.__new__. Currently we mostly use the Coord.... form elsewhere so I would suggest we stick with that.
...or even just DimCoord(...) at the bottom?
As stated by @esc24, avoiding the constructor is the whole point of this PR 😉
clswill beDimCoordat runtime.
True ... unless someone had defined a subclass of DimCoord.
There was a problem hiding this comment.
True ... unless someone had defined a subclass of DimCoord.
Of course.
Why not call DimCoord.__new__() directly rather than the method from its superclass? That should allow for future customisation of DimCoord.__new__() as well as super(DimCoord, cls).__new__() (I'm assuming DimCoord.__new__() will call super(DimCoord, cls).__new__()).
There was a problem hiding this comment.
Why not call DimCoord.new() directly rather than the method from its superclass?
Doh! I'd got the wrong end of the stick before. Now I understand why you mentioned DimCoord.__new__ and cls.__new__. Sorry! 😐
I'll switch it to DimCoord.__new__(cls).
There was a problem hiding this comment.
Presumably the point is to avoid a call to init() with its checks for monotonicity etc.
Hmm, I see, thanks.
ghost
assigned
There was a problem hiding this comment.
@esc24's cls.__new__(cls) is better because it works for subclasses too.
We want SparklyDimCoord.from_regular to return a SparklyDimCoord.
There was a problem hiding this comment.
Except we have no idea what arguments the SparklyDimCoord constructor needs. NB. If SparklyDimCoord wants to override from_regular it still can.
There was a problem hiding this comment.
NB. DimCoord.__new__(cls) will return an instance of SparklyDimCoord ... that's what the cls parameter is for.
There was a problem hiding this comment.
@rhattersley is too polite to point out I was talking nonsense with cls.__new__(cls) 😞
There was a problem hiding this comment.
NB. DimCoord.new(cls) will return an instance of SparklyDimCoord ... that's what the cls parameter is for.
Great, no problem then.
There was a problem hiding this comment.
@rhattersley is too polite to point out I was talking nonsense
@esc24 is too polite to point out that's "we", not "I" 😀
|
This is ready. |
|
...and for https://github.com/SciTools/iris-code-generators/issues/9, of course... |
Merged. |
Switch to using DimCoord.from_regular
4 participants
This PR converts usage of
f.regular_points()within the PP rules into a new factory methodDimCoord.from_regular()(via SciTools/iris-code-generators#9). By controlling the creation of the points and bounds arrays, the factory method can bypass some of the sanity checks and provide a performance boost.Approx 3% performance boost doing iris.load_cubes() on a 17MB PP file containing 2048 fields.