DM-40652: Raise error when data is insufficient #38
Conversation
cmsaunders
force-pushed
the
tickets/DM-40652
branch
from
646275c
to
30a98e8
Compare
| @@ -436,6 +436,11 @@ def run( | |||
| ``outputCatalog`` : `pyarrow.Table` | |||
| Catalog with fit residuals of all sources used. | |||
| """ | |||
| if (len(inputVisitSummaries) == 1) and (self.config.deviceModel and self.config.exposureModel): | |||
| raise RuntimeError( | |||
| "More than one exposure is necessary to break the degeneracy between the" | |||
There was a problem hiding this comment.
| "More than one exposure is necessary to break the degeneracy between the" | |
| "More than one exposure is necessary to break the degeneracy between the " |
| if (len(inputVisitSummaries) == 1) and (self.config.deviceModel and self.config.exposureModel): | ||
| raise RuntimeError( | ||
| "More than one exposure is necessary to break the degeneracy between the" | ||
| " device model and the exposure model." |
There was a problem hiding this comment.
| " device model and the exposure model." | |
| "device model and the exposure model." |
| @@ -436,6 +436,11 @@ def run( | |||
| ``outputCatalog`` : `pyarrow.Table` | |||
| Catalog with fit residuals of all sources used. | |||
| """ | |||
| if (len(inputVisitSummaries) == 1) and (self.config.deviceModel and self.config.exposureModel): | |||
There was a problem hiding this comment.
You could skip the set of parenthesis towards the end of the line.
https://developer.lsst.io/python/style.html#the-minimum-number-of-parentheses-needed-for-correctness-and-readability-should-be-used
There was a problem hiding this comment.
True, I was thinking about self.config.deviceModel and self.config.exposureModel going together, but I realize it's logically the same without parentheses.
| # free parameters in the per-visit mapping. Set minFitExposures to be | ||
| # the number of free parameters, so that visits with fewer visits are | ||
| # dropped. | ||
| nCoeffVisitModel = int(((2 * self.config.exposurePolyOrder + 3) ** 2 - 1) / 8) |
There was a problem hiding this comment.
Could you briefly comment on how this calculation comes about? Even better would be to make this a utility function and comment it as a docstring, since you use this calculation at least twice in this code.
There was a problem hiding this comment.
Ok, I decided to make utility functions for this and for the inverse operation.
| visitMap = wcsf.mapCollection.orderAtoms(f"{visit}")[0] | ||
| visitMapType = wcsf.mapCollection.getMapType(visitMap) | ||
| if (visitMap not in mapParams) and (visitMapType != "Identity"): | ||
| self.log.warning(f"Visit {visit} was dropped because of an insufficient number of sources.") |
There was a problem hiding this comment.
| self.log.warning(f"Visit {visit} was dropped because of an insufficient number of sources.") | |
| self.log.warning("Visit %d was dropped because of an insufficient number of sources.", visit) |
There was a problem hiding this comment.
Can you explain this suggestion? Is there an issue with f-strings in logging, or are you otherwise concerned about formatting of visit?
There was a problem hiding this comment.
f-strings in loggers are frowned upon and non-standard where deferred formatting is the norm. For warnings it doesn't really matter because we can be very sure that the warning will always be issued (whereas for debugs you definitely don't want to use f-strings).
| @@ -948,6 +962,47 @@ class `wcsfit.FoFClass`, associating them into matches as you go. | |||
|
|
|||
| return sourceIndices, columns | |||
|
|
|||
| def _check_degeneracies(self, associations, extensionInfo): | |||
There was a problem hiding this comment.
It looks like you could decorate it as a staticmethod as it doesn't use self in the body.
There was a problem hiding this comment.
It does use a config parameter actually, so I need self.
| if unconstrainedDetectors: | ||
| raise RuntimeError( | ||
| "The model is not constrained. The following detectors do not have enough " | ||
| f"sources ({nCoeffDetectorModel} required): " + ", ".join(unconstrainedDetectors) |
There was a problem hiding this comment.
Because this is within parenthesis, you could drop the + and the strings would concatenate anyway.
cmsaunders
force-pushed
the
tickets/DM-40652
branch
2 times, most recently
from
84cb7b3
to
23368fb
Compare
| nCoeffs : `int` | ||
| Number of coefficients for the polynomial in question. | ||
| """ | ||
| nCoeffs = int(((2 * degree + 3) ** 2 - 1) / 8) |
There was a problem hiding this comment.
This formula wasn't obvious but makes sense now after reading the docstring. Why not just (degree+2)*(degree+1)/2 though? It doesn't seem like the simplest form of the quantity you want to compute.
There was a problem hiding this comment.
Yeah, I don't remember why I didn't simplify this when I first implemented it. I think my idea was to preserve the structure of the formula, but I don't think it's necessary.
cmsaunders
force-pushed
the
tickets/DM-40652
branch
from
23368fb
to
f19fcb5
Compare
No description provided.