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

Update for new DeblendCoaddSourcesTask #62

Merged
merged 2 commits into from Aug 24, 2018
Merged

Update for new DeblendCoaddSourcesTask #62

merged 2 commits into from Aug 24, 2018

Conversation

fred3m
Copy link
Contributor

@fred3m fred3m commented Aug 14, 2018

No description provided.

butler=butler)
measureInputSchema = afwTable.Schema(self.deblendCoaddSources.schema)
else:
measureInputSchema = afwTable.Schema(self.mergeCoaddDetections.schema)
Copy link

@hsinfang hsinfang Aug 14, 2018

Choose a reason for hiding this comment

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

Does this if/else (line 165 and 185) exist to make this new multiBandDriver backward-compatible with older data products generated with an older Stack?

In other words, for running multiBandDriver.py consistently with a post-DM-15104 Stack, shouldn't self.config.measureCoaddSources.inputCatalog.startswith("deblended") always be true?

Choose a reason for hiding this comment

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

On second look, maybe this part was added to make multiBandDriver more configurable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this is not always true. Just like before when it was possible to turn the deblender off with doDeblend, it is now possible to skip deblending using inputCatalog="mergeDet".

self.deblenderOutput.append("deblendedFlux")
if self.measurementInput not in self.deblenderOutput:
err = "Measurement input '{0}' is not in the list of deblender output catalogs '{1}'"
raise ValueError(err.format(self.measurementInput, self.deblenderOutput))

Choose a reason for hiding this comment

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

Do lines 166-178 do more than checking if things are configured correctly? The comment suggested so to me. If so, this whole block can practically be removed, right? I'm a bit uneasy such checking lives in pipe_drivers; it looks like introducing additional logics beyond stitching tasks into a workflow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct that they only check whether or not the output from the deblender is consistent with the input to the measurements. I feel like it's important to check that the tasks are consistent before running them, otherwise the user would have to wait for the entire deblending task to complete before crashing, and the error might not be obvious.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with Fred here, there are may times when people are developing things and run pipe drivers either for the data output or to test code. It is always a shame when something you set up to run over night or over a weekend crashes a few hours in. This test is cheap and really help developers from falling into this trap accidentally.

Copy link
Member

Choose a reason for hiding this comment

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

Could this validation be moved to MultiBandDriverConfig.validate?

The list comprehensions in the mapping functions were using a
variable from above loops instead of the local comprehension
variable. This change fixes that.
@natelust natelust merged commit 77d2039 into master Aug 24, 2018
@ktlim ktlim deleted the tickets/DM-15104 branch August 25, 2018 06:50
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

4 participants