DM-50420: Inject variable sources into the template and science images#275
DM-50420: Inject variable sources into the template and science images#275BrunoSanchez wants to merge 2 commits into
Conversation
BrunoSanchez
force-pushed
the
tickets/DM-50420
branch
from
64963f4 to
25f247c
Compare
BrunoSanchez
changed the title
BrunoSanchez
force-pushed
the
tickets/DM-50420
branch
from
25f247c to
c387c87
Compare
isullivan
left a comment
isullivan
left a comment
There was a problem hiding this comment.
Looks good! A few comments for readability, and potential bugs.
| seed = int(str(visitId)+str(detId)) | ||
| rng = np.random.default_rng(seed) |
There was a problem hiding this comment.
| seed = int(str(visitId)+str(detId)) | |
| rng = np.random.default_rng(seed) | |
| rng = np.random.default_rng([visitId, detId]) |
I am very wary of relying on string concatenation for creating the seed.
|
|
||
| def __init__(self, **kwargs): | ||
| super().__init__(**kwargs) | ||
| self.log = logging.getLogger(__name__) |
There was a problem hiding this comment.
Not needed, this over-writes the inherited logger.
| randMags["mag"] = mags | ||
| return randMags | ||
|
|
||
| def createRandomMagnitudes(self, nFakes, rng): |
There was a problem hiding this comment.
It looks like the only use of createRandomMagnitudes was deleted earlier. Should this method be removed?
| catalog = vstack([catalog, variable_fakes]) | ||
|
|
||
| if len(catalog) > len(np.unique(catalog["injection_id"])): | ||
| self.log.warning("Duplicate injection IDs detected after catalog assembly; reassigning them.") |
There was a problem hiding this comment.
This might be worth raising an error if there are duplicates, instead of warning and reassigning.
| Catalog of sources detected on the calibrated exposure. | ||
| visit_image : `lsst.afw.image.Exposure` | ||
| Visit image to inject synthetic sources into. | ||
| summaryMetrics : `lsst.metrics.MetricMeasurementBundle` |
There was a problem hiding this comment.
summaryMetrics is not a parameter. Delete?
| min=16, | ||
| max=24, |
There was a problem hiding this comment.
I could imagine someone wanting to inject fakes outside of these magnitude ranges. Probably drop the RangeField and min/max
There was a problem hiding this comment.
But we should not allow people to inject values that are none sense right? I think people will want to have a range of magnitudes. I don't know if it is wise to drop the field completely. Maybe increase the range?
There was a problem hiding this comment.
I suppose, but there isn't a physical hard limit that makes sense. If we set limits here, then it is not possible for someone using our pipelines to process data from a different observatory to overwrite the magnitude ranges in the config to anything brighter or fainter.
There was a problem hiding this comment.
What I don't want to is to leave this as non-configurable. How do we do that? Set instead of a RangeField a free value parameter field? Not sure if that's your suggestion here.
There was a problem hiding this comment.
I was just thinking of making it a standard Field, since it has a reasonable default value.
| min=16, | ||
| max=24, | ||
| ) | ||
| magMax = pexConfig.RangeField( |
There was a problem hiding this comment.
As above, probably drop the RangeField
| ymin, ymax = bbox.getMinY(), bbox.getMaxY() | ||
| visit_stats = visit_image.getInfo().getSummaryStats() | ||
|
|
||
| # corners = visit_image.getBBox().getCorners() |
There was a problem hiding this comment.
Delete commented-out code?
| size=n_variable_fakes | ||
| ) | ||
| variable_fakes["mag"] += variable_fakes["mag_offset"] | ||
| variable_fakes["isVisitSource"] = ~variable_fakes["isVisitSource"] |
There was a problem hiding this comment.
Worth a code comment explaining the flag negations here.
|
|
||
| return Struct(outputCat=catalog) | ||
|
|
||
| def select_hosts(self, sourceCat, *args, **kwargs): |
There was a problem hiding this comment.
Why include *args and **kwargs if they're not used?
BrunoSanchez
force-pushed
the
tickets/DM-50420
branch
from
c387c87 to
4efc359
Compare
BrunoSanchez
force-pushed
the
tickets/DM-50420
branch
from
4efc359 to
3c640e3
Compare
2 participants
Create the fake injection pipeline tools for injection on template images.