DM-43615: Add visit summary metric dispatching to CalibrateImageTask #945
Conversation
jrmullaney
requested review from
ctslater and
parejkoj
and removed request for
ctslater
| @@ -41,6 +41,23 @@ | |||
| from . import measurePsf, repair, photoCal, computeExposureSummaryStats, snapCombine | |||
|
|
|||
|
|
|||
| class _EmptyTargetTask(pipeBase.PipelineTask): | |||
There was a problem hiding this comment.
I'm not a fan of this approach. Could those metrics be moved into a different package, to fix the circular import problem? Alternately, could we modify ConfigurableField to allow optional and then let target=None? That seems like a better approach, though it still doesn't solve the problem of not being able to test the new if branch.
There was a problem hiding this comment.
I'm also not a fan, and I like to think this is a temporary workaround the to the circular import problem. The reason we didn't go down the target=None route was to ensure that a meaningful error message was raised if one tried to implement metric-writing without retargeting.
The circular import arises solely due to analysis_tools needing pipe_tasks to be set up to import the LoadReferenceCatalogTask; if that (and colorterms, which LoadReferenceCatalogTask uses) can be moved out of pipe_tasks, then the circular import is fixed. However, I don't know whether that is something that would be considered feasible.
Could those metrics be moved into a different package...?
My understanding is that we want to avoid/move away from having metrics created outside of analysis_tools (is that what you mean?), in order to ensure that the same tool run from within a task or as a standalone analysis tool will produce the same results (from this post by @natelust; apologies to Nate if there is any misunderstanding on my part on this).
I seem to recall @TallJimbo saying that he was "ok" with this (temporary??) workaround, but that's not to say that it shouldn't be reconsidered. It may come down to how critical it is to have metric creation in here for OR4; but that decision is above my pay grade :) .
Again, apologies if I've misrepresented either Nate or Jim.
There was a problem hiding this comment.
What exactly is the circular import problem, anyway? analysis_tools does not depend on pipe_tasks.
There was a problem hiding this comment.
I believe analysis_tools -> cp_pipe -> pipe_tasks is the dependency.
| @@ -41,6 +41,23 @@ | |||
| from . import measurePsf, repair, photoCal, computeExposureSummaryStats, snapCombine | |||
|
|
|||
|
|
|||
| class _EmptyTargetTask(pipeBase.PipelineTask): | |||
There was a problem hiding this comment.
I'm also not a fan, and I like to think this is a temporary workaround the to the circular import problem. The reason we didn't go down the target=None route was to ensure that a meaningful error message was raised if one tried to implement metric-writing without retargeting.
The circular import arises solely due to analysis_tools needing pipe_tasks to be set up to import the LoadReferenceCatalogTask; if that (and colorterms, which LoadReferenceCatalogTask uses) can be moved out of pipe_tasks, then the circular import is fixed. However, I don't know whether that is something that would be considered feasible.
Could those metrics be moved into a different package...?
My understanding is that we want to avoid/move away from having metrics created outside of analysis_tools (is that what you mean?), in order to ensure that the same tool run from within a task or as a standalone analysis tool will produce the same results (from this post by @natelust; apologies to Nate if there is any misunderstanding on my part on this).
I seem to recall @TallJimbo saying that he was "ok" with this (temporary??) workaround, but that's not to say that it shouldn't be reconsidered. It may come down to how critical it is to have metric creation in here for OR4; but that decision is above my pay grade :) .
Again, apologies if I've misrepresented either Nate or Jim.
| @@ -41,6 +41,23 @@ | |||
| from . import measurePsf, repair, photoCal, computeExposureSummaryStats, snapCombine | |||
|
|
|||
|
|
|||
| class _EmptyTargetTask(pipeBase.PipelineTask): | |||
There was a problem hiding this comment.
I believe analysis_tools -> cp_pipe -> pipe_tasks is the dependency.
jrmullaney
force-pushed
the
tickets/DM-43615
branch
from
f78e4ec
to
dab6a8a
Compare
jrmullaney
force-pushed
the
tickets/DM-43615
branch
from
dab6a8a
to
68bbba5
Compare
No description provided.