Conversation
| hmax = 1.5 if np.nanquantile(fwhm, 0.75) < 1.5 else 3.0 | ||
| axs[0, 2].hist(fwhm, bins=np.linspace(0.4, hmax, 100), color="C0") |
There was a problem hiding this comment.
Is it OK to hard-code the 0.4, 100 here and below?
There was a problem hiding this comment.
I think it's fine. We can give the magic numbers some names if you like. Part of the goal for this PR though is to have consistent plots from one visit to the next - hence the fixed limits.
|
|
||
| # Kurtosis hist | ||
| axs[2].hist(kurtosis, bins=int(np.sqrt(len(table))), color="C3") | ||
| axs[2].hist(kurtosis, bins=np.linspace(1.8, 2.3, 100), color="C3") |
There was a problem hiding this comment.
Is it OK to hard-code these too?
| table: Table, | ||
| camera: Camera, | ||
| maxPointsPerDetector: int = 5, | ||
| band: str, |
There was a problem hiding this comment.
I think you're actually passing in the physical filter, not the band, so it might be better to call it that. In fact, it's annotated as filter below, too.
There was a problem hiding this comment.
Yeah, I switched this from filter since that's a python builtin. Happy to make this physical_filter though.
| filter : `str` | ||
| The filter name to include in the plot title. | ||
| camera : `list` | ||
| The list of camera detector objects. | ||
| band : `str` | ||
| Name of the current filter band. | ||
| maxPointsPerDetector : `int`, optional |
There was a problem hiding this comment.
One of these needs to go.
| table: Table, | ||
| camera: Camera, | ||
| maxPointsPerDetector: int = 5, | ||
| band: str, |
There was a problem hiding this comment.
See previous comment about band vs filter.
| # azelAngle = 0.0 | ||
| # xyAngle = -np.pi / 2 - table.meta["rotTelPos"] | ||
| # neAngle = -table.meta["rotTelPos"] - table.meta["rotSkyPos"] |
There was a problem hiding this comment.
Please remove commented code.
| axs: npt.NDArray[np.object_], | ||
| table: Table, | ||
| camera: Camera, | ||
| band: str, |
There was a problem hiding this comment.
See previous comment about band vs filter.
| filter : `str` | ||
| The filter name to include in the plot title. | ||
| camera : `list` | ||
| The list of camera detector objects. | ||
| band : `str` | ||
| Name of the current filter band. |
There was a problem hiding this comment.
Again, one needs removing.
| plotHigherOrderMomentsData(axs[2, :], table, prefix="aa_") | ||
|
|
||
| plotData(axs[:2, :], table, maxPointsPerDetector, minPointsPerDetector, prefix="aa_") | ||
| band = band[0] # "i_39" -> "i" for example |
There was a problem hiding this comment.
- I'm not sure this is robust, and 2) I think this makes it clear that it should be
band = filter[0].
There was a problem hiding this comment.
I went ahead with band = physical_filter[0].
| "u": "#85b7ff", | ||
| "g": "#a4dfaf", | ||
| "r": "#e28d7e", | ||
| "i": "#ffe07e", | ||
| "z": "#f89fd0", | ||
| "y": "#ad7f7f", |
jmeyers314
force-pushed
the
tickets/DM-54262
branch
from
18185f1 to
5834009
Compare
|
Thanks @mfisherlevine . Think I've hit all your comments. |
3 participants
No description provided.