tickets/dm-33992 Support generating Pipeline to include in DRP pipe #20

natelust · 2022-03-14T17:15:07Z

Draft PR of the complete pipeline building code

TallJimbo

Main comment here is that I don't see any separation of visit-level vs. coadd-level vs. survery-level plots. Based on the conversation yesterday, I expected to see three executions of the builder in the SConscript, each reading in and producing a different pipeline, with each of those defining a different labeled subset for all of their pipelines. And then pipelines in drp_pipe would include those three machine-generated pipelines directly.

I think you could achieve the same result in terms of subset definitions using the pattern-matching functionality you've added here, while running the builder only once, given @sr525 's statement about plot names always having suffixes like "_visit" or "_coadd", and maybe I misunderstood the plan from the meeting about how to achieve having those subsets. But I think we need to make them somehow.

I'm going to point the Slack channel at this comment to make sure I've understood the plan for splitting up plots correctly.

TallJimbo · 2022-03-16T14:39:35Z

pipelines/generic_plots.in

+  - "$ANALYSIS_DRP_DIR/pipelines/visitQAEllipPlots.yaml"
+  - "$ANALYSIS_DRP_DIR/pipelines/visitQAPlots.yaml"
+
+description: "A pipeline including all analysis drp plots"


Add newline at end of file.

TallJimbo · 2022-03-16T14:39:57Z

pipelines/SConscript

+
+targets.setdefault(
+    "pipelines",
+    []).extend(


This is pretty unusual formatting; I'd put a newline between ] and ) with no indent, but that's mostly just a guess at what black would do.

this is what black outputted for me, I will run it again once all changes are done.

TallJimbo · 2022-03-16T14:41:41Z

python/lsst/analysis/drp/pipelineBuilder.py

+        self.parser = argparse.ArgumentParser()
+        self.parser.add_argument("pipeline", help="input pipeline to process")
+
+    def getGroups(self) -> Mapping[str, tuple[Optional[str], Iterable[str]]]:


If this is about defining subsets, maybe just call it "subsets" rather than "groups"?

TallJimbo · 2022-03-16T14:42:56Z

python/lsst/analysis/drp/pipelineBuilder.py

+    def run(self, args: Sequence[str]) -> None:
+        """Main entry point for actually building a pipeline. This is
+        responsible for parsing the supplied args (likely coming from the
+        command line, and then calling the build method.


Suggested change

command line, and then calling the build method.

command line), and then calling the build method.

And whenever talking about args from the command-line, I think it's important to clarify whether it expects sys.argv or sys.argv[1:].

TallJimbo · 2022-03-16T14:43:37Z

python/lsst/analysis/drp/pipelineBuilder.py

+
+        Subsets are added to the output pipeline that contain all the plots
+        (except plots contained in the exclude set), and any other subsets
+        defined within the builders ``getGroups`` method.


Suggested change

defined within the builders ``getGroups`` method.

defined within the builder's ``getGroups`` method.

TallJimbo · 2022-03-16T14:44:03Z

python/lsst/analysis/drp/pipelineBuilder.py

+        ValueError
+            Raised if a subset is already declared with a name returned from
+            the ``getGroups`` method.
+            Raise if a subset is alreday declared with a name that conflicts


Suggested change

Raise if a subset is alreday declared with a name that conflicts

Raised if a subset is already declared with a name that conflicts

TallJimbo · 2022-03-16T15:08:23Z

pipelines/generic_plots.in

+  - "$ANALYSIS_DRP_DIR/pipelines/makeQaTractTables.yaml"
+  - "$ANALYSIS_DRP_DIR/pipelines/stellarLocusPlots.yaml"
+  - "$ANALYSIS_DRP_DIR/pipelines/visitQAEllipPlots.yaml"
+  - "$ANALYSIS_DRP_DIR/pipelines/visitQAPlots.yaml"


Here's a concrete proposal for how to split this up:

all_coadd_plots.yaml.in: imports coaddQAPlots.yaml and stellarLocusPlots.yaml

all_visit_plots.yaml.in: imports visitQAPlots.yaml and visitQAEllipPlots.yaml

all_survey_plots.yaml.in: empty for now.

That looks good to me, though are @erykoff's plots in a separate pipeline as well?

Mine can't be in a generic pipeline because they can't run generically... how that all fits in together I definitely don't understand.

I am leaving his pipeline as a completely independent thing, that anything needs to directly refer to (especially as it seems to only be able to be included in one place right now if I understand correctly) we can modify that on future tickets.

There is also going to be matchVisitCatalogs.yaml->all_visit_plots.yaml.in and matchCoaddCatalogs.yaml->all_coadd_plots.yaml.in as soon as Jenkins finishes for my PR.

i think it's better for a pipeline to self-declare where it can be used, when possible, via this kind of structure. Having a flat directory with tons of plot pipelines, in which some are secretly but knowingly instrument-specific, seems like a trap.

Unless they declare an instrument, because clashes in instruments are not allowed. It seems like that is how a pipeline is supposed to declare itself as belonging to an instrument rather than a position on a filesystem, or at the very least it should be both. As I said I am not invested enough to push bash on either.

Adding the instrument to the pipeline seems like it's worth doing as well, but without some mechanism for conditionally importing pipelines that looks at the instrument, I don't think it solves the problem.

I'm not sure I follow why conditional anything is important.

Sorry, I guess I jumped a few steps without explaining myself. I am thinking about how to try to solve the problem of "I am a downstream pipeline author; how do I know which upstream plot-pipelines to include in my pipeline?" Having the instrument inside the pipeline but no relevant organizational structure for the pipelines themselves only seems to help with that problem if there is a way they can say, "include all of the pipelines but automatically filter out the ones with the wrong instrument".

This patch introduces a python module which builds a top level generic plotting pipeline containing plots which are generally appropriate for inclusion in most drp processing pipelines. Scons by default will build the pipeline, and the resulting pipeline will be ignored by git.

Make sure visit level plots, and coadd level plots have unique parameter names so they do not trample each other when combined into one pipeline.

This patch works around some failures in scatterPlot.py until a the underlying issues can be resolved.

TallJimbo · 2022-04-05T14:59:18Z

python/lsst/analysis/drp/scatterPlot.py

+            toPlotList = [(catPlot[xCol].values, catPlot[yCol].values, "purple", None,
+                           sourceTypeMapper["all"])]
+        else:
+            toPlotList = []


On your follow-up ticket, please also request code comments in and around this block, as I have no idea what it's doing, but I think that's not a job for you.

TallJimbo · 2022-04-05T15:01:16Z

python/lsst/analysis/drp/scatterPlot.py

-        fig = self.scatterPlotWithTwoHists(plotDf, plotInfo, sumStats)
+        try:
+            fig = self.scatterPlotWithTwoHists(plotDf, plotInfo, sumStats)
+        except Exception:


As we discussed, just make sure we have a follow-up ticket to make this go away.

natelust force-pushed the tickets/DM-33992 branch 4 times, most recently from 5c625ee to 25a691b Compare March 16, 2022 14:19

natelust changed the title ~~Draft tickets/dm-33992~~ tickets/dm-33992 Support generating Pipeline to include in DRP pipe Mar 16, 2022

TallJimbo approved these changes Mar 16, 2022

View reviewed changes

TallJimbo reviewed Mar 16, 2022

View reviewed changes

natelust force-pushed the tickets/DM-33992 branch 3 times, most recently from 5acf636 to ea65d68 Compare March 22, 2022 19:06

natelust force-pushed the tickets/DM-33992 branch 2 times, most recently from 71ee2a7 to dae5fda Compare March 27, 2022 18:49

natelust added 3 commits March 27, 2022 13:50

Add explicit dependencies in pipeline building

b7dfa82

Use unique parameter names in Pipelines

88eb1d9

Make sure visit level plots, and coadd level plots have unique parameter names so they do not trample each other when combined into one pipeline.

natelust force-pushed the tickets/DM-33992 branch 2 times, most recently from d8c2bd1 to 88eb1d9 Compare March 30, 2022 14:08

natelust added 5 commits March 31, 2022 12:05

Remove imSim plots pending bugfix

7c38da7

Workaround issue when serializing configs

001e2ec

Work around Zero bin size

ee090bc

Re-enable plots for dc2 coaddition

b66497a

Works around failures in scatterPlot

a1e9cbf

This patch works around some failures in scatterPlot.py until a the underlying issues can be resolved.

natelust force-pushed the tickets/DM-33992 branch from 78d987f to a1e9cbf Compare April 4, 2022 21:39

TallJimbo approved these changes Apr 5, 2022

View reviewed changes

natelust merged commit ac18fba into main Apr 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tickets/dm-33992 Support generating Pipeline to include in DRP pipe #20

tickets/dm-33992 Support generating Pipeline to include in DRP pipe #20

natelust commented Mar 14, 2022

TallJimbo left a comment •

edited

TallJimbo Mar 16, 2022

TallJimbo Mar 16, 2022

natelust Mar 16, 2022 •

edited

TallJimbo Mar 16, 2022

TallJimbo Mar 16, 2022

TallJimbo Mar 16, 2022

TallJimbo Mar 16, 2022

TallJimbo Mar 16, 2022 •

edited by arunkannawadi

sr525 Mar 16, 2022

erykoff Mar 16, 2022

natelust Mar 16, 2022

cmsaunders Mar 16, 2022

TallJimbo Mar 16, 2022 •

edited

natelust Mar 16, 2022

TallJimbo Mar 16, 2022

natelust Mar 16, 2022

TallJimbo Mar 16, 2022

TallJimbo Apr 5, 2022

TallJimbo Apr 5, 2022

	command line, and then calling the build method.
	command line), and then calling the build method.

	defined within the builders ``getGroups`` method.
	defined within the builder's ``getGroups`` method.

	Raise if a subset is alreday declared with a name that conflicts
	Raised if a subset is already declared with a name that conflicts

tickets/dm-33992 Support generating Pipeline to include in DRP pipe #20

tickets/dm-33992 Support generating Pipeline to include in DRP pipe #20

Conversation

natelust commented Mar 14, 2022

TallJimbo left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natelust Mar 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TallJimbo Mar 16, 2022 • edited by arunkannawadi

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TallJimbo Mar 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TallJimbo left a comment •

edited

natelust Mar 16, 2022 •

edited

TallJimbo Mar 16, 2022 •

edited by arunkannawadi

TallJimbo Mar 16, 2022 •

edited