DM-22478: Lightly restructure ap_association tasks and create new DiaObject/DiaSource pre-loading task. #67
Conversation
There was a problem hiding this comment.
I have questions about whether some bits are necessary, but things look good overall.
However, in the future, please break up the work into well-defined commits. It would have made your changes much easier to follow if I could see what was and wasn't related to what.
| diaSourceHistory, | ||
| exposure, | ||
| apdb): | ||
| """Load associate the new DiaSources with existing or new DiaObjects |
| diaObjects : `pandas.DataFrame` | ||
| Existing diaObjects from the Apdb. | ||
| diaSourceHistory : `pandas.DataFrame` | ||
| 12 month DiaSource history of the loaded ``diaObjects``. | ||
| exposure : `lsst.afw.image` | ||
| Input exposure representing the region of the sky the dia_sources | ||
| were detected on. Should contain both the solved WCS and a bounding |
There was a problem hiding this comment.
Can't comment quite far enough down, but your description of apdb says:
Apdb connection object to retrieve DIASources/Objects from and write to.
Should this be only "write to" now?
| # Now that we know the DiaObjects our new DiaSources are associated | ||
| # with, we index the new DiaSources the same way as the full history | ||
| # and merge the tables. | ||
| diaSources.set_index(["diaObjectId", "filterName", "diaSourceId"], | ||
| drop=False, | ||
| inplace=True) |
There was a problem hiding this comment.
It's a bit confusing to do this after calling apdb.storeDiaSources. Does that mean that the in-memory index is irrelevant when calling storeDiaSources (not just in terms of using it directly, but things like clustering and access speed).
There was a problem hiding this comment.
It is, this is just for fast indexing in pandas.
| dia_object_record["decl"], | ||
| geom.degrees)) | ||
| return bbox.contains(point) | ||
|
|
||
| def check_dia_souce_radec(self, dia_sources): |
There was a problem hiding this comment.
Rename to check_dia_source_radec?
| def update_dia_objects(self, dia_objects, updated_obj_ids, exposure, apdb): | ||
| def update_dia_objects(self, | ||
| dia_objects, | ||
| diaSourceHistory, | ||
| updated_obj_ids, | ||
| exposure, | ||
| apdb): |
There was a problem hiding this comment.
If you're reformatting to a more list-like style, consider going all the way so that it's easier to remove/append parameters:
exposure,
apdb,
)
There was a problem hiding this comment.
The formatting you suggest throws a linting error. Seems to be because it's a function definition not a function call.
There was a problem hiding this comment.
It works for me as long as the closing parenthesis is below the "s" in self. I don't think I'll ever understand all the rules of Python formatting 😵
tests/test_loadDiaCatalogs.py
Outdated
| # CFHT Filters from the camera mapper. | ||
| self.filter_names = ["u", "g", "r", "i", "z"] | ||
| afwImageUtils.resetFilters() | ||
| afwImageUtils.defineFilter('u', lambdaEff=374, alias="u.MP9301") | ||
| afwImageUtils.defineFilter('g', lambdaEff=487, alias="g.MP9401") | ||
| afwImageUtils.defineFilter('r', lambdaEff=628, alias="r.MP9601") | ||
| afwImageUtils.defineFilter('i', lambdaEff=778, alias="i.MP9701") | ||
| afwImageUtils.defineFilter('z', lambdaEff=1170, alias="z.MP9801") |
There was a problem hiding this comment.
I don't think you need this bit. Assuming your test exposures do need a filter, afwImage.Filter('g', force=True) would skip all the usual validation.
There was a problem hiding this comment.
Removed the extra filters. Don't really see the need to worry about the extra validation of the filter so I'll leave it as is.
|
|
||
| # Expected HTM pixel ranges for max range=4 and level = 20. This | ||
| # set of pixels should be same for the WCS created by default in | ||
| # makeExposure and for one with a flipped y axis. |
There was a problem hiding this comment.
And what happens if you have a flipped x axis?
There was a problem hiding this comment.
Moves it to a different place on the sky but does not invert the direction the polygon is traveled.
There was a problem hiding this comment.
As in changing said values doesn't test what I want which is that right and left handed polygons (as in going around the bounding box points clockwise or counter clockwise) produce the same results.
tests/test_loadDiaCatalogs.py
Outdated
| self.ranges = np.sort( | ||
| np.array([(15154776375296, 15154779521024), | ||
| (15154788958208, 15154792103936)]).flatten()) |
There was a problem hiding this comment.
This seems really roundabout: you're creating a 2D array, then flattening it? Why not create a vector right away?
There was a problem hiding this comment.
Mostly because of the way I produced and copied the numbers into the code. Have changed it.
tests/test_loadDiaCatalogs.py
Outdated
| self.exposure, | ||
| self.pixelator) | ||
|
|
||
| self.dateTime = dafBase.DateTime("2014-05-13T17:00:00.000000000", |
There was a problem hiding this comment.
Code duplication; why not pass the DateTime into makeExposure?
There was a problem hiding this comment.
Now grab it from the exposure.
| Also check that they can be properly loaded both by location and | ||
| ``diaObjectId``. |
There was a problem hiding this comment.
It might be better to make these two separate tests, just to make sure there's no cross-talk.
Remove DiaObject/Source loading from association. Change sphgeom HTM indexer. Debug HTM diaCalc plugin. Add mock Apdb to DiaForced unittest. Change to sphgeom indexer. Change update_dia_objects unittest. Add htmIndex calculation to unittest. Debug update diaObjects. Edit association run method unittest. Debug test_run. Debug all association tests. Create loadDiaCatalogs tests. Add getPixelRanges test. Add unittest for getPixelRanges Debug getPixelRanges test. Add object, source, and run tests. debug loadDiaCatalogs unittests. Add comment to diaCaluclation. Add HTMIndex validation test. Respond to reviewer comments. Fix typos throughout. Modify loadDiaCatalogs test to byPixelId and byObjId separately. Remove some duplicated code. Fix missed name change. Style changes from review.
morriscb
force-pushed
the
tickets/DM-22478
branch
from
b2f1265
to
252aa7d
Compare
Restructure ap_association to accomidate preloading.
Remove DiaObject/Source loading from association.
Change sphgeom HTM indexer.
Debug HTM diaCalc plugin.
Add mock Apdb to DiaForced unittest.
Change to sphgeom indexer.
Change update_dia_objects unittest.
Add htmIndex calculation to unittest.
Debug update diaObjects.
Edit association run method unittest.
Debug test_run.
Debug all association tests.
Create loadDiaCatalogs tests.
Add getPixelRanges test.
Add unittest for getPixelRanges
Debug getPixelRanges test.
Add object, source, and run tests.
debug loadDiaCatalogs unittests.
Add comment to diaCaluclation.