Skip to content
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.

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

DM-30386: Make DRP wrapper task for DiaCalculation task #116

Merged
merged 3 commits into from Jun 24, 2021

Conversation

morriscb
Copy link
Contributor

No description provided.

@@ -167,7 +167,7 @@ def run(self,
diaObjects,
mergedDiaSourceHistory,
matchResult.associated_dia_object_ids,
filterName)
[filterName])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question here: how are the filter name(s) passed? They're not already a list, are they? If I look at e.g. python/lsst/pipe/tasks/drpDiaCalculationPipe.py you have

self.config.filterNames

passed, so it's read from the config. In the actual config is it something like "u","g","r",... as opposed to ["u", "g", ...] my only concern was that you might unexpectedly get a list of lists somehow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filterName here comes from the diaSource catalog itself and will always be a string value here. If you look a few lines above this, it is where filterName is pulled. The pipeline here will only ever be run over one filter as it is designed to run in AP that is one ccdVisit at time. This one runs in the DiaPipeTask pipeline task vs the new wrapper.

Copy link
Contributor

@kherner kherner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question on the deleted diaCalculation.py and diaCalculationPlugins.py files. When I compare the lines in the "new" versions in meas_base I see there are 514 and 1,110 lines, respectively, but 463 and 1,093 in the now-deleted versions. The test files are the same though. Do you know off the top of your head where the difference come from? I cna do a detailed comparison but if this expected maybe we can save some time.

@morriscb
Copy link
Contributor Author

It's due to 1) the extra for loop over plugins outside of the one that loops over filters first. 2) adding the needsFilter boolean to each plugin.

@morriscb morriscb merged commit 11ff3f0 into master Jun 24, 2021
@morriscb morriscb deleted the tickets/DM-30386 branch June 24, 2021 04:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants