DM-9190: Cleanup pybind11 remaining code #14
Conversation
pschella
force-pushed
the
tickets/DM-9190
branch
4 times, most recently
from
8c72604
to
5d50c96
Compare
python/lsst/shapelet/tractor.py
Outdated
| import lsst.pex.exceptions | ||
| from .radialProfile import RadialProfile | ||
| from .multiShapeletBasis import MultiShapeletBasis | ||
| from .shapeletFunction import ShapeletFunction |
There was a problem hiding this comment.
Please consider cleaning up two pre-existing issues:
try:
import pickle as pickle
except ImportError:
import pickle
should be simplified to import pickle (I am guessing futurize made this bit of silliness).
standard_library.install_aliases()
needs a # noqa to make flake8 happy.
Also as a heads up: this plotSuite is almost certainly broken because y is not defined when first used. I filed DM-9976 about it.
| class ShapeletFunction: | ||
| def __reduce__(self): | ||
| return (ShapeletFunction, (self.getOrder(), self.getBasisType(), | ||
| self.getEllipse(), self.getCoefficients())) |
There was a problem hiding this comment.
This and all other new xContinued.py functions contain trivial flake8 errors; please clean them up:
- a flake8 complaint about redefining the class; I assume a
# noqa F811will be required to quell this warning. - add an extra blank line before
@continueclass - remove the extra blank line at the end (a final carriage return is wanted, but not two of them)
autopep8will clean up the last two items.
There was a problem hiding this comment.
That said...would it make any sense to turn this into C++ code? Or is that just too much work for right now (since this ticket is huge). Perhaps consider filing a ticket to define __reduce__ in C++ pretty much everywhere? It seems a shame to add this whole subdirectory structure when C++ could do the job.
There was a problem hiding this comment.
It is possible. But probably not worth it for now. We eventually need some utility code for that I think.
| class RadialProfile: | ||
| def evaluate(self, r): | ||
| if isinstance(r, np.ndarray): | ||
| return self._evaluate(r.ravel()).reshape(r.shape) |
There was a problem hiding this comment.
Interesting that Python accepts n-dimensional data and C++ does not. But presumably we rely on that. Pity the C++ isn't more flexible so we could skip this. I realize a lambda function could handle this, but that would take significantly more code than we have here.
| # Without this fails with: TypeError: lsst.shapelet.constants.BasisTypeEnum.__new__( | ||
| # lsst.shapelet.constants.BasisTypeEnum) is not safe, use object.__new__() | ||
| def __reduce__(self): | ||
| return (BasisTypeEnum, (int(self), )) |
There was a problem hiding this comment.
Any idea if this is still needed? (I'm not sure how old the note is)
There was a problem hiding this comment.
yes, this is (unfortunately) still needed.
pschella
force-pushed
the
tickets/DM-9190
branch
from
5d50c96
to
2588cd4
Compare
pschella
force-pushed
the
tickets/DM-9190
branch
from
2588cd4
to
a90a174
Compare
No description provided.