DM-32450: Add camera geometry plotting utilities #131
Conversation
|
|
||
| xAxisLabel = Field[str](doc="Label to use for the x axis.", optional=False) | ||
| yAxisLabel = Field[str](doc="Label to use for the y axis.", optional=False) | ||
| zAxisLabel = Field[str](doc="Label to use for the z axis.", optional=False) |
There was a problem hiding this comment.
Not sure, but can these and statistic below just be inherited from the parent class?
There was a problem hiding this comment.
I believe so. I've removed them and confirmed that the plots look identical.
|
|
||
| # U/V coordinates represent focal plane locations. | ||
| minU, minV = corners.min(axis=0) | ||
| maxU, maxV = corners.max(axis=0) |
There was a problem hiding this comment.
Can you just use these to set plotLimit_x and plotLimit_y rather than go through the loop above?
| # This does the appropriate statistic for this | ||
| # detector's data. | ||
| statistic, _, _ = binned_statistic_dd( | ||
| [focalPlane_x[detectorInd], focalPlane_y[detectorInd]], |
There was a problem hiding this comment.
Aren't focalPlane_x and focalPlane_y all zeros here? I guess it doesn't matter, since you're just trying to get one number for the whole detector, but then it seems like you could use something simpler than binned_statistic_dd. That said, I couldn't find another scipy function with the needed functionality.
There was a problem hiding this comment.
The contents of focalPlane_xy are arbitrary, and should be ignored in this block. I also tried to find another function that understood the same statistic argument, and tracked through the source to see that the other binned_statistic functions make calls to binned_statistic_dd, which does the conversion from the string statistic name to the numeric result itself (here: https://github.com/scipy/scipy/blob/v1.11.1/scipy/stats/_binned_statistic.py#L596).
| ampName = amplifier.getName() | ||
| detectorInd = data["detector"] == detectorId | ||
| ampInd = data["x"] == ampName | ||
| detectorInd &= ampInd |
There was a problem hiding this comment.
Personally, I think it would be clearer to reverse this to ampInd &= detectorInd so that you are using ampInd later.
|
|
||
| class FocalPlaneGeometryPlot(FocalPlanePlot): | ||
| """Plots the focal plane distribution of a parameter in camera | ||
| geometry units. |
There was a problem hiding this comment.
Can you clarify that camera geometry units means detectors or amplifiers?
| for amplifier in detector: | ||
| ampName = amplifier.getName() | ||
| detectorInd = data["detector"] == detectorId | ||
| ampInd = data["x"] == ampName |
There was a problem hiding this comment.
If x is supposed to be the amp name, can you add that to the docs?
There was a problem hiding this comment.
This comment convinced me to switch from using x (which as suggested, isn't obvious) to a new amplifier field. I've updated the docs to reflect this. This also allows the x and y vectors to be ignored, allowing a single catalog to be plotted using either FocalPlanePlot or FocalPlaneGeometryPlot (for level='detector').
I wrote a method to manage some further conversions, but it seemed out of scope for this ticket, so I've saved that as a snippet that might be useful in the future.
* Switch to an explicit 'amplifier' column, so as not to overload 'x'. * Switch to using 'z' column for all expected lengths, as it should always exist. * Clarify index name, as it applies to amplifiers, not detectors.
No description provided.