DM-10777: Create TransformBoundedField #254
Conversation
| // -*- LSST-C++ -*- | ||
| /* | ||
| * LSST Data Management System | ||
| * Copyright 2017 LSST Corporation. |
There was a problem hiding this comment.
Shouldn't this be different? A year range or so?
There was a problem hiding this comment.
It starts out as the year of authorship. That is changed to a range for future edits in a later year.
There was a problem hiding this comment.
The template used to be different. But no problem, just curious.
There was a problem hiding this comment.
It should be different — not because of the date, but because the copyright isn't held by LSST Corporation. Our usual convention is to use "Copyright NNNN AURA/LSST" (which is likely also incorrect, but at least it's in the template...).
| /** | ||
| * Create a TransformBoundedField from a bounding box and transform. | ||
| */ | ||
| TransformBoundedField(geom::Box2I const& bbox, Transform const& transform); |
There was a problem hiding this comment.
Add explicitly default (or deleted) copy and move constructors / assignment operators?
There was a problem hiding this comment.
Good point. I'll emulate Transform, which allows copying but not assignment. That said, I'm not certain if that's the best choice, and if you have an alternate suggestion let me know.
| Transform getTransform() const { return _transform; } | ||
|
|
||
| /// @copydoc BoundedField::evaluate | ||
| virtual double evaluate(geom::Point2D const & position) const override; |
There was a problem hiding this comment.
No need for the virtual here (clear from override) and leaving it out is what the style guide recommends (I think).
|
|
||
| /// @copydoc BoundedField::evaluate | ||
| virtual ndarray::Array<double, 1, 1> evaluate(ndarray::Array<double const, 1> const& x, | ||
| ndarray::Array<double const, 1> const& y) const override; |
There was a problem hiding this comment.
Same here, but this overload is not virtual in the base class.
There was a problem hiding this comment.
That was true before this ticket. I changed the array version of BoundedField::evaluate to virtual on this ticket, for this reason.
| using BoundedField::evaluate; | ||
|
|
||
| /// TransformBoundedField is always persistable. | ||
| virtual bool isPersistable() const override { return true; } |
tests/test_transformBoundedField.py
Outdated
| import numpy as np | ||
| from numpy.testing import assert_allclose | ||
|
|
||
|
|
| lsst.afw.geom.Extent2I(5, 30)) | ||
|
|
||
| # a list of points contained in the bbox | ||
| self.pointList = lsst.afw.geom.Box2D(self.bbox).getCorners() |
There was a problem hiding this comment.
Are corners still considered to be in the box?
There was a problem hiding this comment.
I believe so. In this case it doesn't matter -- the functional form of the transform has no intrinsic limits; the bbox is arbitrary.
| # applylForward returns an array with one row of values per axis | ||
| # and in this case there is just one axis | ||
| predResArr = self.transform.applyForward(self.pointList)[0] | ||
| assert_allclose(resArr, predResArr) |
There was a problem hiding this comment.
BoundedField.evaluate does not checks limits, so such a test is not helpful.
tests/test_transformBoundedField.py
Outdated
| assert_allclose(scaledField1.evaluate(self.xList, self.yList), predResult) | ||
|
|
||
| scaledField2 = multFactor * self.boundedField | ||
| assert_allclose(scaledField2.evaluate(self.xList, self.yList), predResult) |
There was a problem hiding this comment.
What about very large, very small, negative or otherwise problematic or invalid multiplication factors?
There was a problem hiding this comment.
I'll add a few values. I don't want to try to overflow or anything like that, but a reasonble dynamic range of values seems wise.
tests/test_transformBoundedField.py
Outdated
|
|
||
| self.assertTrue(self.boundedField == readField) | ||
| self.assertFalse(self.boundedField != readField) | ||
| self.assertEqual(self.boundedField, readField) |
There was a problem hiding this comment.
Move equality tests up before the previous block.
r-owen
force-pushed
the
tickets/DM-10777
branch
2 times, most recently
from
e60b664
to
3a26ea4
Compare
include/lsst/afw/geom/SkyWcs.h
Outdated
| void writeStream(std::ostream & os) const override; | ||
|
|
||
| /// Serialize this SkyWcs to a string, using the same format as writeStream | ||
| virtual std::string writeString() const; |
| } | ||
|
|
||
| return Transform(*frameSet); | ||
| } |
|
|
||
| template<class Transform> | ||
| void writeStream(Transform const & transform, std::ostream & os) { | ||
| os << "1 " << Transform::getShortClassName(); |
There was a problem hiding this comment.
Please add a serializationVersion (or other name, perhaps just version) constexpr for the version magic number.
There was a problem hiding this comment.
OK. Given that I the unpersistence code is in the file, and has to use hard-coded numbers, I don't see a lot of gain, but I guess it beats writing an int as a string. I added serializationVersion as a constexpr int.
r-owen
force-pushed
the
tickets/DM-10777
branch
6 times, most recently
from
7468dc2
to
b3f722f
Compare
The set/unset characteristic of the Base and Current attributes of the FrameSet were not being preserved by all of Transform's constructors, causing unwanted failures in testing equality of transforms (e.g. after persisting and unpersisting).
This will be used for persistence of Transform and to simplify the pybind11 wrapper of Transform
and use it to simplify Transform's pybind11 wrapper. This will also be used by Transform::readStream and writeStream
Add Transform and SkyWcs serialization methods readStream, readString, writeStream and writeString. Only the string versions are available in Python, since C++ streams are not easy to wrap. The work is done by two new utility functions in transformUtils.h so the code can be shared between Transform and SkyWcs. Update unit tests accordingly.
Given the new Transform persistence methods, there is no need for saveToFile.
and change it to read a string, not a file. That matches the new persistence better and is more general-purpose
Make `BoundedField::evaluate(ndarray::Array...` virtual so `TransformBoundedField` can override it with a more efficient implementation.
Move the ChebyshevBoundedFieldFactory constructor to the beginning of the class methods and make it explicit.
and update the ChebyshevBoundedField wrapper accordingly. Wrap `__ne__` for `BoundedField` to eliminate the need to wrap it in subclasses. This was especially nasty because the C++ method `operator!=` was only defined in `BoundedField`, so it was easy to miss in wrappers of all subclasses, and failure to wrap it resulted in errors that only showed up in Python 2, not Python 3. Wrap `__eq__` for `BoundedField`, even though it has no implementation. This is best practice and may eliminate the need to wrap it in subclasses, but I'm not going to test that. Wrap `__rmul__` for BoundedField to eliminate the need to wrap it in subclasses. Update the `ChebyshevBoundedField` pybind11 wrapper accordingly: remove the wrapper for `__ne__` and `__rmul__`.
No description provided.