New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DM-30024: Implement matching in DRP DiaAssociation task #526

Merged

morriscb merged 4 commits into master from tickets/DM-30024

Jun 9, 2021

Contributor

morriscb commented Jun 2, 2021

No description provided.

morriscb force-pushed the tickets/DM-30024 branch 2 times, most recently from 47a81e5 to fa60904 Compare

June 2, 2021 21:39

yalsayyad reviewed

View reviewed changes

Contributor

yalsayyad left a comment

And squash the "fixup comments" commit.

python/lsst/pipe/tasks/associationUtils.py

		@@ -0,0 +1,79 @@
		import healpy as hp

Contributor

yalsayyad Jun 9, 2021

https://developer.lsst.io/python/style.html?highlight=order#standard-code-order-should-be-followed

Add license statement
module-docstring
imports

Contributor Author

morriscb Jun 9, 2021

This set functions was copied over from the LSSTDESC/dia_pipe directory. Didn't want worry about getting this up to spec for now. I can make a ticket to document it up to standards.

python/lsst/pipe/tasks/associationUtils.py



		def eq2vec(ra, dec):
		"""Convert equatorial ra,dec in degrees to x,y,z on the unit sphere parameters"""

Contributor

yalsayyad Jun 9, 2021

I wouldn't have been able to guess just by the name that this is the vectorized version of eq2xyz. Maybe eq2xyzVec or eqVec2xyz

Contributor Author

morriscb Jun 9, 2021

See above.

python/lsst/pipe/tasks/drpAssociationPipe.py Outdated

@@ @@ -85,17 +90,33 @@ class DrpAssociationPipeTask(pipeBase.PipelineTask): @@
                   """
                   ConfigClass = DrpAssociationPipeConfig
                   _DefaultName = "drpAssociation"
+                  RunnerClass = pipeBase.ButlerInitializedTaskRunner

Contributor

yalsayyad Jun 9, 2021

This is a Gen3-only task, right? RunnerClasses are only needed for Gen2

python/lsst/pipe/tasks/drpAssociationPipe.py Outdated

		@@ -161,13 +182,19 @@ def run(self, diaSourceTables, skyMap, tractId, patchId):
		# self.associator.addCatalog()

Contributor

yalsayyad Jun 9, 2021

I know this wasn't added on this changeset, but it's gotta go eventually

python/lsst/pipe/tasks/simpleAssociation.py

+                      ----------
+                      diaSrc : `pandas.Series`
+                          Full unassociated DiaSource to create a DiaObject from.
+                      diaSources : `pandas.DataFrame`

Contributor

yalsayyad Jun 9, 2021

This is the object that's modified in place, right? Would be clearer to me if the parameter descriptions explicitly said who is modified

python/lsst/pipe/tasks/simpleAssociation.py Outdated

+                                     diaObjCat,
+                                     diaObjCoords,
+                                     healPixIndices):
+                      """Create a new DiaObject and append its data.

Contributor

yalsayyad Jun 9, 2021

This doc string is the same as addNewDiaObject. I'm skeptical.

python/lsst/pipe/tasks/simpleAssociation.py Outdated

+                                        "nearbyObj2": 0,
+                                        "nearbyObj3": 0,
+                                        "flags": 0}
+                      for f in ["u", "g", "r", "i", "z", "y"]:

Contributor

yalsayyad Jun 9, 2021

It'd be worth making the band list a config parameter.

Contributor Author

morriscb Jun 9, 2021

I don't need this here actually. This is all needed in the Apdb as these are columns that aren't nullable.

python/lsst/pipe/tasks/simpleAssociation.py Outdated

+                      Parameters
+                      ----------
+                      diaSources : `pandas.DataFrame`
+                          DiaSources to spatially associate into DiaObjects

Contributor

yalsayyad Jun 9, 2021

Could say a little more about the format of the diaSources it's expecting. Like is it one big concatenation of all the visits? What does it group by and loop over? ccdVisitId?

python/lsst/pipe/tasks/simpleAssociation.py Outdated

+                  Represents a simple, brute force algorithm matching DiaSources into
+                  DiaObjects. Algorithm picks the nearest, first match DiaObject to
+                  associate a source to.

Contributor

yalsayyad Jun 9, 2021

Could say a little more about how it works. Like I'm assuming it loops over visits and 2-way matches each visit's diaSourceTable to the modified diaObject table.

Contributor

sr525 Jun 9, 2021

What does nearest first match mean? Does it stop checking once it finds one within some radius? Or check everything and take the nearest?

Contributor Author

morriscb Jun 9, 2021

First match is "the fist match found withing radius". I'll update the comment a bit.

sr525 reviewed

View reviewed changes

tests/test_simpleAssociation.py

+                      self.hpIndices = [toIndex(simpleAssoc.config.nside,
+                                                diaObj["ra"],
+                                                diaObj["decl"])

Contributor

sr525 Jun 9, 2021

Why do you use a mixture of decl and dec as abbreviations?

Contributor Author

morriscb Jun 9, 2021

decl is the column name in the DPDD and our Apdb schema, hence use of it.

morriscb added 3 commits

June 9, 2021 11:49


          Add utilities and simple_association task.

d559cc9

Finish docs and rename to camel case.


          Add unittest.

7cdf8ae

Debug SimpleAssociationTask.

Debug add and update unittests.

Debug unittests.

Add run test.

switch variables to camel case.

Debug SimpleAssociationTask variable names.

Task worked with very little proding. I'm good at my job :)

Add full run test.

Debug simple association run test.


          Hook up SimpleAssociation into DrpAssociationPipeline.

b926ef6

Debug pipeline and linting fixes.

Debug pipeline.

Confim outputs on ap_verify ci datasets.

Fixup docs.

Update docs on test_simpleAssociation.py

Add quick comment to associationUtils.py.

Fixup comments.

morriscb force-pushed the tickets/DM-30024 branch from 3a28745 to 3106f82 Compare

June 9, 2021 16:49

sr525 reviewed

View reviewed changes

tests/test_simpleAssociation.py Outdated

+                      self.diaObjDecs = np.linspace(45, 46, self.nDiaObjects)
+                      # Copy a coord to get multiple matches.
+                      self.diaObjRas[3] = self.diaObjRas[2] + 0.1 / 3600
+                      self.diaObjDecs[3] = self.diaObjDecs[2] + 0.1 / 3600

Contributor

sr525 Jun 9, 2021

Formatting. No spaces around /.

tests/test_simpleAssociation.py

+                           "ra": ra,
+                           "decl": decl}
+                          for idx, (ra, decl) in enumerate(zip(self.diaObjRas,
+                                                               self.diaObjDecs))]

Contributor

sr525 Jun 9, 2021

This is another example of formatting that I find hard to follow.

Contributor Author

morriscb Jun 9, 2021

So, since it's linting and passing standards I'm likely just going to keep it this way.


          Respond to reviewers.

26f8951

Add drpAssociation to diffImDRP subset.

Fix linting.

Fix white space around division.

morriscb force-pushed the tickets/DM-30024 branch from ee62333 to 26f8951 Compare

June 9, 2021 19:47

morriscb merged commit e1a9e10 into master

morriscb deleted the tickets/DM-30024 branch

June 9, 2021 22:26

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment