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-36208: Add a README to analysis_tools #107

Merged
merged 1 commit into from May 16, 2023
Merged

Conversation

leeskelvin
Copy link
Contributor

No description provided.

Copy link
Contributor

@parejkoj parejkoj left a comment

Choose a reason for hiding this comment

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

A couple of suggestions. Thanks for expanding this.

README.rst Outdated

``analysis_tools`` is a package in the `LSST Science Pipelines <https://pipelines.lsst.io>`_.
The Analysis Tools package is designed to assist in the creation of quality assurance (QA) plots and metrics from the outputs of a data reduction pipeline.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The Analysis Tools package is designed to assist in the creation of quality assurance (QA) plots and metrics from the outputs of a data reduction pipeline.
The Analysis Tools package is designed to assist in the creation of quality assurance (QA) plots and metrics from the outputs of a PipelineTask.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use of the phrase "data reduction pipeline" here was specific, as data derived from more than just a single pipeline task may be used to generate plots and metrics. I can't think of a neater way to summarize that than "data reduction pipeline".

Copy link
Contributor

Choose a reason for hiding this comment

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

"data reduction pipeline" is either too specific (not just DRP!) or not specific enough (it has to be one of our LSST Pipelines). Maybe "from the outputs of an LSST Pipeline" and link the latter two to https://pipelines.lsst.io/modules/lsst.pipe.base/creating-a-pipeline.html? I don't know if we have a better summary page for what we mean when we say "Pipeline", but it's a more specific thing than what non-LSST people mean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DRP != Data Reduction Pipeline.
DRP = Data Release Production.

Does that change your thinking? I think the phrase "data reduction pipeline" is sufficiently instantly recognisable as to what we mean here - namely, a pipeline that has reduced data.

Happy to add a URL link.

Copy link
Contributor

Choose a reason for hiding this comment

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

The link helps. We are specifically talking about our capital-P Pipelines, and that makes it clear.

README.rst Outdated
``analysis_tools`` is a package in the `LSST Science Pipelines <https://pipelines.lsst.io>`_.
The Analysis Tools package is designed to assist in the creation of quality assurance (QA) plots and metrics from the outputs of a data reduction pipeline.
The intention is that users have the flexibility to construct complex data analysis tasks from a set of simple building blocks.
In this sense, a series of consistent, repeatable and high-quality plots and metrics can be generated for any given dataset.
Copy link
Contributor

Choose a reason for hiding this comment

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

"In this sense"? Maybe something like this:

Suggested change
In this sense, a series of consistent, repeatable and high-quality plots and metrics can be generated for any given dataset.
Analysis Tools allows users to generate consistent, repeatable, and high-quality plots and metrics on the output of a PipelineTask, no matter which dataset it is run on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Barring the pipeline task issue above, this is 👍

@leeskelvin leeskelvin force-pushed the tickets/DM-36208 branch 2 times, most recently from 66e8a70 to 871e7f6 Compare May 16, 2023 15:42
README.rst Outdated
``analysis_tools`` is a package in the `LSST Science Pipelines <https://pipelines.lsst.io>`_.
The Analysis Tools package is designed to assist in the creation of quality assurance (QA) plots and metrics from the outputs of an `LSST data reduction pipeline <https://pipelines.lsst.io/modules/lsst.pipe.base/creating-a-pipeline.html>`_.
The intention is that users have the flexibility to construct complex data analysis tasks from a set of simple building blocks.
These analysis tools therefore allow users to generate a series of consistent, repeatable and high-quality plots and metrics, regardless of which dataset they are run on.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the "therefore" is necessary.

Suggested change
These analysis tools therefore allow users to generate a series of consistent, repeatable and high-quality plots and metrics, regardless of which dataset they are run on.
Analysis tools allows users to generate a series of consistent, repeatable and high-quality plots and metrics, regardless of which dataset they are run on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It helps if you read the fully rendered paragraph, I find: https://github.com/lsst/analysis_tools/tree/tickets/DM-36208.

If sentence 1 in the paragraph begins "The Analysis Tools package..." and then sentence 3 begins "Analysis tools...", it's too much repetition of the phrase in quick succession. This is why I initially preferred "In that sense...", as it's clear were still discussing in the context of sentence 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use of "therefore" is an attempt to break up that repetition in an attempt to satisfy your comment.

@leeskelvin leeskelvin force-pushed the tickets/DM-36208 branch 2 times, most recently from 4240ebb to 27a4381 Compare May 16, 2023 16:02
@leeskelvin
Copy link
Contributor Author

PS - as Analysis Tools isn't yet linked in the static docs, I've added an additional sentence linking to the current daily build. I think this will be useful in the coming weeks, as in-kind collaborators begin adding code and wanting to reference what we have right now.

@parejkoj
Copy link
Contributor

Good point on linking to the daily docs build: I think that's probably a good idea generally (at least until we have an official data release), since the docs are evolving pretty fast right now.

@leeskelvin leeskelvin merged commit 8968b92 into main May 16, 2023
7 checks passed
@leeskelvin leeskelvin deleted the tickets/DM-36208 branch May 16, 2023 19:25
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