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-28429: Convert MultiProFitTask into gen3 PipelineTask #474

Merged
merged 1 commit into from Mar 3, 2021

Conversation

taranu
Copy link
Contributor

@taranu taranu commented Mar 1, 2021

No description provided.

if bands_extra:
for dref in ref_d:
if dref.dataId['band'] in bands_extra:
ref_d.remove(dref)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added logic here to drop the exposure dataref from bands only in bands_read_only, but then I remembered that I actually need the photoCalib to calibrate the catalog for magnitude-based priors.

Would it be possible to do that and add a photoCalib dataref that isn't specified in the connections, or is that beyond what adjustQuantum is meant to do? (I'm probably not going to do it either way as it over-complicates things for a marginal benefit - the exposure object doesn't actually read fits images into memory until you first retrieve them, right?)



class MultibandFitSubConfig(pexConfig.Config):
def bands_read_only(self) -> Set:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't make this an ABC, but I wanted a default implementation anyway. PyCharm complains that this could be a staticmethod - adding that wouldn't affect subclasses' overrides, would it?

]
dataIds = set(cats).union(set(exps))
catexps = [
CatalogExposure(catalog=cats.get(dataId), exposure=exps.get(dataId), dataId=dataId)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that CatalogExposures' catalogs and exposures are optional (I can see future use cases for just wanting one or the other attached to a dataId), I went with defensively putting inputs into dicts as you suggested and in case future changes to valid subtasks decide to drop one or the other. Does that seem reasonable?

@taranu taranu merged commit f0327e3 into master Mar 3, 2021
@taranu taranu deleted the tickets/DM-28429 branch March 3, 2021 22:20
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

1 participant