DM-31619: Fully incorporate SSP object association in DiaPipe #132
Conversation
| doSolarSystemAssociation = pexConfig.Field( | ||
| dtype=bool, | ||
| default=False, | ||
| doc="", |
There was a problem hiding this comment.
While I think the name is pretty self-explanatory here please do give this a non-null docstring.
| for idx, diaSource in unAssocDiaSources.iterrows(): | ||
| newDiaObjectsList.append( | ||
| self._initialize_dia_object(diaSource["diaSourceId"])) | ||
| unAssocDiaSources.loc[idx, "diaObjectId"] = diaSource["diaSourceId"] |
There was a problem hiding this comment.
I'm confused here as to why you are setting the diaObjectId to be the diaSourceId. Are these not distinct ids?
There was a problem hiding this comment.
In any case this doesn't need to be looped: you can simply say
unAssocDiaSources['diaObjectId'] = unAssocDiaSources['diaSourceId']
There was a problem hiding this comment.
The id for newly created DiaObjects has been the same as the id of the first DiaSource that makes up the object since we've had DiaObjects in the pipeline. It's an easy way to give all the DiaObjects a unique id.
tests/test_diaPipe.py
Outdated
| def testRun(self): | ||
| """Test running while creating and packaging alerts. | ||
| """ | ||
| self._testRun(True, True) |
There was a problem hiding this comment.
| self._testRun(True, True) | |
| self._testRun(doPackageAlerts=True, doSolarSystemAssociation=True) |
tests/test_diaPipe.py
Outdated
| def testRunWithSolarSystemAssociation(self): | ||
| """Test running while creating and packaging alerts. | ||
| """ | ||
| self._testRun(False, True) |
There was a problem hiding this comment.
| self._testRun(False, True) | |
| self._testRun(doPackageAlerts=False, doSolarSystemAssociation=True) |
tests/test_diaPipe.py
Outdated
| def testRunWithAlerts(self): | ||
| """Test running while creating and packaging alerts. | ||
| """ | ||
| self._testRun(True) | ||
| self._testRun(True, False) |
There was a problem hiding this comment.
| self._testRun(True, False) | |
| self._testRun(doPackageAlerts=True, doSolarSystemAssociation=False) |
tests/test_diaPipe.py
Outdated
| """Test running without creating and packaging alerts. | ||
| """ | ||
| self._testRun(False) | ||
| self._testRun(False, False) |
There was a problem hiding this comment.
| self._testRun(False, False) | |
| self._testRun(doPackageAlerts=False, doSolarSystemAssociation=False) |
| diaSources.loc[idx, "diaObjectId"] = diaSource["diaSourceId"] | ||
| for idx, diaSource in unAssocDiaSources.iterrows(): | ||
| newDiaObjectsList.append( | ||
| self._initialize_dia_object(diaSource["diaSourceId"])) |
There was a problem hiding this comment.
I would investigate if you can run this without the iterrows loop, for example:
newDiaObjectsSeries = unAssocDiaSources['diaSourceId'].apply(self._initialize_dia_object)
which if it works will likely be faster than the explicit loop.
There was a problem hiding this comment.
I'll give it a shot.
There was a problem hiding this comment.
Seems to work. I remember apply being only slightly faster than iterrows when not using built in pandas operations. Still, it's likely to be a bit faster since it now operates on one column not the whole row.
Change outputs of AssociationTask slightly. Debug line break.
Fixup unittest and datetime in skybotQuery. Use reset_index to copy sub DataFrames. Change to MagicMock Debug unittests.
morriscb
force-pushed
the
tickets/DM-31619
branch
2 times, most recently
from
f049426
to
1300354
Compare
Change new dia object creation behavior. Remove superfluous log statement.
morriscb
force-pushed
the
tickets/DM-31619
branch
from
6f3b1c4
to
2d97f39
Compare
No description provided.