DM-12272: Fix bug in arrayFromVector #30
Conversation
| @@ -43,7 +43,7 @@ using Array2D = ndarray::Array<double, 2, 2>; | |||
| /** | |||
| 2D array of const double; typically used for lists of const points | |||
| */ | |||
| using ConstArray2D = ndarray::Array<double const, 2, 2>; | |||
| using ConstArray2D = ndarray::Array<const double, 2, 2>; | |||
There was a problem hiding this comment.
The new way is inconsistent with LSST convention.
There was a problem hiding this comment.
In this case one reads left to right; a similar issue comes up for shared_ptr, e.g. "shared_ptr" is a shared pointer to a const foo. I know we've adopted this convention in real life, but I don't know if it's in the standard.
There was a problem hiding this comment.
Which standard do you mean? 😕
include/astshim/base.h
Outdated
| @@ -78,6 +78,11 @@ Reshape a vector as a 2-dimensional array that shares the same memory | |||
| @warning You must hold onto the original vector until you are done | |||
| with the returned array, else the array will be corrupted. | |||
There was a problem hiding this comment.
Per the style guide, notes should appear after the parameter/return/exception section.
| @@ -67,14 +67,14 @@ ConstArray2D arrayFromVector(std::vector<double> const &vec, int nAxes) { | |||
| } | |||
|
|
|||
| Array2D arrayFromVector(std::vector<double> &vec, int nAxes) { | |||
| if (vec.size() % nAxes != 0) { | |||
| int nPoints = vec.size() / nAxes; | |||
| if (nPoints * nAxes != vec.size()) { | |||
There was a problem hiding this comment.
I don't understand this change. What's wrong with the original test?
There was a problem hiding this comment.
I felt the test was more obvious the new way. I'd seen it done elsewhere in our stack and I preferred it.
tests/test_base.py
Outdated
| def test_arrayFromVector(self): | ||
| nAxes = 3 | ||
| nValues = 5 | ||
| dataVec = np.random.rand(nAxes * nValues) |
There was a problem hiding this comment.
Good catch. In this case I don't see how it can make any difference, but providing a seed is best practice.
tests/test_base.py
Outdated
| dataVec = np.random.rand(nAxes * nValues) | ||
| desiredDataArr = dataVec.copy() | ||
| desiredDataArr.shape = (nAxes, nValues) | ||
| dataArr = ast.arrayFromVector(vec=dataVec, nAxes=nAxes) |
There was a problem hiding this comment.
Isn't dataVec a numpy.ndarray? Would it be useful to test this with a list?
There was a problem hiding this comment.
I agree. I'll test both ways.
r-owen
force-pushed
the
tickets/DM-12272
branch
from
18183c3
to
f203c09
Compare
the strides vector was incorrect in arrayFromVector. Add a pybind11 wrapper and a unit test.
explaining how to use it for a vector of polynomial coefficients and move the warning to after the parameter list as per our standard
I was only using the typedefs in a few locations (basically for data, but not coefficient arrays). I think the intent is clearer using the typedefs everwhere, especially given that the coefficient array arguments should have had const elements, but did not.
r-owen
force-pushed
the
tickets/DM-12272
branch
from
f203c09
to
4035ac0
Compare
No description provided.