DM-48826 : add SSO association to drpAssociationPipe #1040
Conversation
TallJimbo
changed the title
| @@ -193,3 +193,71 @@ def query_disc(nside, ra, dec, max_rad, min_rad=0): | |||
| pixels = pixels[match] | |||
|
|
|||
| return pixels | |||
|
|
|||
|
|
|||
| def objID_to_ssObjectID(objID, flags=0): | |||
There was a problem hiding this comment.
Our naming conventions would write this as
obj_id_to_ss_object_id
or
objIdToSsObjectId
(same for other symbols in this file)
| dimensions=("instrument", "visit", "detector"), | ||
| deferLoad=True, | ||
| multiple=True | ||
| ) |
There was a problem hiding this comment.
I would prefer for these (visitInfo, bbox, wcs) to all be replaced by a single finalVisitSummary input, as I think that'll be a lot less I/O overall and pull in the best WCS instead of one we would not normally use at this stage (and in the future, might not want to keep on disk at this point in the processing).
Another possibility would be to use pvi.<component> instead of calexp.<component>, as pvi will be available and will have the final WCS, but I think using finalVisitSummary is still slightly better.
There was a problem hiding this comment.
That's better. I hadn't realized finalVisitSummary contained all those.
|
|
||
| if not config.doSolarSystemAssociation: | ||
| self.inputs.remove("solarSystemObjectTables") | ||
| self.outputs.remove("associatedSsSources") |
There was a problem hiding this comment.
Should unassociatedSsObjects be removed here, too?
I'd also recommend slightly different syntax for this, just because it's a bit easier to read (what you have is not incorrect):
| self.outputs.remove("associatedSsSources") | |
| del self.associatedSsSources |
There was a problem hiding this comment.
Had I used your syntax, I would have noticed that I renamed solarSystemObjectTables into ssObjectTableRefs. Changed!
| name="preloaded_DRP_SsObjects", | ||
| storageClass="ArrowAstropy", | ||
| dimensions=("instrument", "visit", "detector"), | ||
| minimum=0, |
There was a problem hiding this comment.
I like this solution, if it means what I think it means; if you run this task, with doSolarSystemAssociation=True, but in a repo with no ephemerides for some or all of the visits, it just skips doing the solar system associations, and does regular DIAObject association only? Does it warn?
| objID : `str` | ||
| Minor Planet Center packed provisional designation for a small solar | ||
| system object. Must be fewer than eight characters. | ||
| flags : `int`, optional |
There was a problem hiding this comment.
I think a name other than flags would be preferable here; elsewhere in pipelines flags correspond to errors, whereas this is more similar to a set of release or version bits.
Gerenjie
force-pushed
the
tickets/DM-48826
branch
3 times, most recently
from
0506c10 to
58d2e36
Compare
Gerenjie
force-pushed
the
tickets/DM-48826
branch
from
58d2e36 to
5fdf5b1
Compare
No description provided.