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-29348: Add ability to run global fgcmcal in a single pipeline. #59

Merged
merged 1 commit into from Apr 14, 2021

Conversation

erykoff
Copy link
Contributor

@erykoff erykoff commented Apr 12, 2021

FgcmFitCycleTask has been updated to allow multiple fit cycles to be run at once. This, in turn, allows the full fgcmcal (build stars; multiple fit cycles; output photoCalibs) to be run as a single pipeline. The gen3 tests now share a common test repo.

@erykoff erykoff force-pushed the tickets/DM-29348 branch 3 times, most recently from 50094d7 to dd0c327 Compare April 13, 2021 22:41
@MorganSchmitz MorganSchmitz self-requested a review April 14, 2021 11:35
Copy link

@MorganSchmitz MorganSchmitz left a comment

Choose a reason for hiding this comment

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

Looks good! I have a few small questions purely for my own curiosity.

Sorry this took a while, I did learn a lot though!

# Run multiple cycles at once.
config = copy.copy(self.config)
config.update(cycleNumber=0)
for cycle in range(self.config.multipleCyclesFinalCycleNumber + 1):

Choose a reason for hiding this comment

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

So we will be doing multipleCyclesFinalCycleNumber+1 cycles, right? Since initial cycle is number 0?

I'm guessing the reason we want the min field in the config to be 2 is that we have at least one non-initial, non-final cycle?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the initial cycle is 0, that is done with limited parameters free. The final cycle (as you say, n+1) is a clean-up with no parameters free. So that makes the min field 2. I'll add comments/docs to this effect.

@@ -85,31 +85,31 @@ class FgcmOutputProductsConnections(pipeBase.PipelineTaskConnections,
deferLoad=True,
)

fgcmVisitCatalog = connectionTypes.PrerequisiteInput(
fgcmVisitCatalog = connectionTypes.Input(

Choose a reason for hiding this comment

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

Why are we changing all these to just Inputs? As far as I can tell, some are still always used (unlike eg fgcmAtmosphereParameters that may not be needed depending on the config) here and in fgcmFitCycle...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously, the three tasks (build stars; fit; output products) all had to be done separately. And I believe a PrerequisiteInput is faster for the the quantum graph generator to find (or not find). But now I have the ability to link them all into one pipeline, so the fgcmVisitCatalog doesn't actually exist prior to the start of the pipeline run, so it must be a regular Input that is constrained by the quantum graph generator.

Comment on lines -413 to -420
fgcmBuildStarsConfig = butlerQC.get(inputRefs.fgcmBuildStarsTableConfig)
physicalFilterMap = fgcmBuildStarsConfig.physicalFilterMap

if self.config.doComposeWcsJacobian and not fgcmBuildStarsConfig.doApplyWcsJacobian:
raise RuntimeError("Cannot compose the WCS jacobian if it hasn't been applied "
"in fgcmBuildStarsTask.")
if not self.config.doComposeWcsJacobian and fgcmBuildStarsConfig.doApplyWcsJacobian:
self.log.warn("Jacobian was applied in build-stars but doComposeWcsJacobian is not set.")

Choose a reason for hiding this comment

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

Why were these checks removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We seem to be unable to read in the configs as regular inputs, and these checks seemed extraneous. But my intention is to add these to the gen3 pipeline "contracts". The pipeline itself isn't anywhere yet since it doesn't quite fit into the current framework; I'll ticket that separately.

@@ -1,12 +1,6 @@
import lsst.fgcmcal as fgcmcal

config.outfileBase = 'TestFgcm'
# The unused z-band is here to test the case that extra filters

Choose a reason for hiding this comment

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

We're still getting that extra-filter test with HSC_FILTER_DEFINITIONS.physical_to_band, right?

FGCM can now be configured to run the full pipeline (build
stars, fit multiple cycles, and output) in one run.  The gen3
testing has also been refactored to use the same repository for
all the tests.
@erykoff erykoff merged commit 543e7c3 into master Apr 14, 2021
@erykoff erykoff deleted the tickets/DM-29348 branch April 14, 2021 21:39
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