DM-25748: Create pipeline task to generate fakes for AP processing and completeness tests #58
Conversation
Debug unittests. Fix arctan calculation. Fix area calculation.
Add a bit more docs. Debug code and unittest.
python/lsst/ap/pipe/createApFakes.py
Outdated
|
|
||
| Returns | ||
| ------- | ||
| randoms : `lsst.pipe.tasks.PaquertTable` |
There was a problem hiding this comment.
It's actually returning DataFrame now so I will change it.
python/lsst/ap/pipe/createApFakes.py
Outdated
| Catalog of random points covering the given tract. Follows the | ||
| format expected in `lsst.pipe.tasks.InsertFakes`. | ||
| """ | ||
| np.random.seed(self.config.randomSeed) |
There was a problem hiding this comment.
Use a np.random generator object instance instead of the built in random state. rnd_gen = np.random.default_rng(seed) etc. Add a note to the seed docstring that this goes to the default_rng().
| ---------- | ||
| nFakes : `int` | ||
| Number of fakes to create. | ||
| boundingCicle : `lsst.sphgeom.BoundingCircle` |
There was a problem hiding this comment.
As far as I can see, all the random generation and transformation support up to a full sphere (up to Pi opening angle). Be explicit in the docstring about the maximum size of the allowed BoundingCircle.
There was a problem hiding this comment.
A larger than Pi value for a bounding circle doesn't make sense as a bounding circle of Pi already encompasses the full sphere.
| Number of fakes to create. | ||
| boundingCicle : `lsst.sphgeom.BoundingCircle` | ||
| Circle bound covering the tract. | ||
|
|
There was a problem hiding this comment.
Having a geometrical distribution of points can easily be ambiguous. Please add a Note in the docstring here and perhaps in the Task description as well that "spatially uniform" actually means a uniform distribution of points per solid angle here.
There was a problem hiding this comment.
added "on the sphere" to be clearer.
| cross /= np.sqrt(cross[0] ** 2 + cross[1] ** 2 + cross[2] ** 2) | ||
| cosTheta = center.z() | ||
| sinTheta = np.sin(np.arccos(center.z())) | ||
|
|
There was a problem hiding this comment.
I did not check that all the terms are correct in this matrix though I see it is tested with random angles (up to a rotation of ?); add a reference in a comment/docstring if this form of the rotation matrix is readily written down somewhere as a formula (I mean textbook, paper, WolframMathWorld, your notebook, tech note etc.).
There was a problem hiding this comment.
Wikipedia baby... https://en.wikipedia.org/wiki/Rotation_matrix the "Rotation from axis and angle" sub heading.
python/lsst/ap/pipe/createApFakes.py
Outdated
| return {self.config.visitSourceFlagCol: isVisitSource, | ||
| self.config.templateSourceFlagCol: isTemplateSource} | ||
|
|
||
| def createRandomMagnitudes(self, nFakes): |
There was a problem hiding this comment.
Please emhpasize here (createUniformRandomMagnitudes()) and note in the Task docstring that this is uniform in magnitudes which is a non-physical model but perhaps all right for this task.
There was a problem hiding this comment.
I kind of want to leave this as a generic name so that it can be overwritten in possible inherited tasks.
Fix linting.
morriscb
force-pushed
the
tickets/DM-25748
branch
from
b28abb4
to
068f394
Compare
No description provided.