DM-12370: Add piecewise TransmissionCurve for coadds. #102
Conversation
TallJimbo
force-pushed
the
tickets/DM-12370
branch
3 times, most recently
from
e2500f3
to
ed8e4b8
Compare
| namespace lsst { namespace meas { namespace algorithms { | ||
|
|
||
| PYBIND11_PLUGIN(coaddTransmissionCurve) { | ||
| py::module mod("cr"); |
There was a problem hiding this comment.
This is a short and not very descriptive module name. It is not clear what will be found within.
There was a problem hiding this comment.
It's actually an incorrect module name. Will fix.
|
|
||
| Parameters | ||
| ---------- | ||
| rng : numpy.random.RandomState |
There was a problem hiding this comment.
Should you specify this needs to a a discrete distribution? Otherwise subtracting off the 0.5 in the perturbed function might not do what you think.
There was a problem hiding this comment.
RandomState is a class with different methods for different distributions; it can always do both discrete and continuous, depending on the method called.
There was a problem hiding this comment.
yes, but then what is the meaning of subtracting off the 0.5, as it seems to be just shifting the central tendency of the distribution. I though you had it to somehow "center" the integer draws of the discrete pdf
There was a problem hiding this comment.
I'm not actually using a discrete distribution; rand() returns a uniform random number between 0 and 1; this transforms it into a random number between -0.5 and 0.5. I them multiply that by 2 to get a distribution between -1 and 1, and then scale by the given pertubation amount.
| (boost::format("No TransmissionCurve for input with ID %d") % record.getId()).str() | ||
| ); | ||
| } | ||
| if (record.getWcs() == nullptr) { |
There was a problem hiding this comment.
consider putting in a check for if the object has a wcs, there might be some weird situation from python were someone would put in a record without a wcs, and this would cause a segfault
There was a problem hiding this comment.
That is exactly what this does; this is the C++ equivalent to if record.getWcs() is None.
src/CoaddTransmissionCurve.cc
Outdated
| (boost::format("No Wcs for input with ID %d") % record.getId()).str() | ||
| ); | ||
| } | ||
| // Set wavelength bounds from min of mins, max of maxes. |
There was a problem hiding this comment.
Add more of a comment explaining that this is to find the min max out of all the input records, this is not clear from variable names. Alternatively clarify with different names perhapse
src/CoaddTransmissionCurve.cc
Outdated
| _wavelengthBounds.first = std::min(_wavelengthBounds.first, inputWavelengthBounds.first); | ||
| _wavelengthBounds.second = std::max(_wavelengthBounds.second, inputWavelengthBounds.second); | ||
| // Set throughput at bounds from weighted average of throughput at bounds. | ||
| auto const inputThroughputAtBounds = transmission->getThroughputAtBounds(); |
There was a problem hiding this comment.
I don't quite get this next part. If the bounds are different for each of the inputs, and you are finding the maximum extents of the bounds above, how are you figuring out the throughput as a linear combination of all the endpoints below. Isn't the throughput dependent on the wavelength bound itself? Shouldn't you only add the throughput if the bounds correspond?
There was a problem hiding this comment.
The values returned by getThroughputAtBounds are guaranteed to be the values at any wavelength outside the range specified by getWavelengthBounds (i.e. I could have called it getThroughputAtAndOutsideBounds).
src/CoaddTransmissionCurve.cc
Outdated
| continue; | ||
| } | ||
| input.transmission->sampleAt(inputPosition, wavelengths, tmp); | ||
| tmp.deep() *= input.weight; |
There was a problem hiding this comment.
Won't this always be zero? the array is defined as zero in all elements above, and you are using an in-place operator here...
There was a problem hiding this comment.
In the line above, tmp is filled by sampleAt (it's an output argument).
TallJimbo
force-pushed
the
tickets/DM-12370
branch
from
16b33e3
to
241757f
Compare
No description provided.