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-38962: Update docs to new API #91

Merged
merged 6 commits into from May 10, 2023
Merged

DM-38962: Update docs to new API #91

merged 6 commits into from May 10, 2023

Conversation

sr525
Copy link
Collaborator

@sr525 sr525 commented May 4, 2023

No description provided.


**contexts**

| Generic settings to be applied in a given circumstance. For example overrides that are specific to a coadd or visit level plot/metric such as the default flag selector which is different between coadd and visit analysis.

**interfaces**

| Interfaces are the framework level code which is used as a basis to build/interact with analysis tools package. You should not have to modify anything in here to be able to add new metrics or plots.
Copy link
Contributor

Choose a reason for hiding this comment

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

These feel like definition lists, but aren't being printed as such because of the leading |. If you start this paragraph with a 4-space indent, this will instead display as a definition, making this and all other entries easier to read.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was an intentional layout choice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But if you like the other way I have no strong opinions and have changed it.

Comment on lines +368 to +414
The current actions that are available are detailed :doc:`here<action-types>`. Most common requests are already coded up and
please try to reuse actions that already exist before making your own. Please also try to make actions as
reusable as possible so that other people can also use them.
Copy link
Contributor

Choose a reason for hiding this comment

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

General comment: if you structure your ReST with the philosophy of one sentence per line, it will make future edits easier to parse.

Comment on lines 16 to 17
4. The command line --config and --config-file options
5. The pipeline file configs
Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding is that 4 and 5 should be switched here. I.e., anything you give on the command line at pipetask run time will always take precedence.

I think this block may be missing a few options as well. Let me ping @natelust to weigh in.


Due to this configs that are obs package specific should not be specified in the pipeline file because they
overwrite the obs specific configs. For example the bands option, if that is set in the pipeline file it will
overwrite the obs package which means that if you try to apply the same pipeline to hsc and DC2 data (for
Copy link
Contributor

Choose a reason for hiding this comment

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

Uppercase HSC

The various flag selectors allow you to specify which bands the flags are applied in. If you want the selector
to be applied in the bands that the plot is being made in then the flag config option should be set to [].

Where is the line between python and yaml?
Copy link
Contributor

Choose a reason for hiding this comment

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

Uppercase Python, uppercase YAML.

Copy link
Contributor

Choose a reason for hiding this comment

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

(here and everywhere)

@@ -76,9 +76,6 @@ def getInputSchema(self) -> KeyedDataSchema:
def __call__(self, data: KeyedData, **kwargs) -> Vector:
"""Select on the given flags

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this belong on this ticket? It seems like a selectors fix, but is just removing Parameters without replacing it. From what I can tell, this looks like it needs data at least?

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 this file needs the word guide in the name. We don't have it for getting-started, and I think it adds unnecessary length. I also imagine this page being less of a guide and more of a reference, so there's that too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a better focus for this page would be FAQs. I.e.: doc/lsst.analysis.tools/faqs.rst. Each block is phrased as a question, and we then periodically update this page as new frequently asked questions emerge.

@leeskelvin
Copy link
Contributor

Thanks Sophie, comments above. Happy to go ahead with this thereafter and build upon it.

@sr525 sr525 force-pushed the tickets/DM-38962 branch 2 times, most recently from 0832fd4 to ea393f0 Compare May 9, 2023 19:16
@sr525 sr525 merged commit 012a343 into main May 10, 2023
7 checks passed
@sr525 sr525 deleted the tickets/DM-38962 branch May 10, 2023 04:16
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