DM-24913: obs_decam README is confusing #196

mrawls · 2021-07-03T01:02:58Z

This PR fixes several outstanding obs_decam issues. Most of them are documentation-related — the README is no longer confusing because it is very short and points to the actual package docs, which are now slightly less awful.

The other big change is a default switch I have been meaning to make for months: from cpBias and cpFlat as the default DECam bias and flat types to just the stack-wide default of bias and flat. This broke tests, which I fixed.

Once merged, I plan to make a Community post all about DECam highlighting this breaking change for community pipeline Gen2 users, and point to the new starter directions I wrote for setting up one's very own DECam-flavored Gen3 repository using raws.

parejkoj

Have you looked at the built docs, including in the full pipelines doc build?

The "using-obs-decam" file looks mostly generic: a lot of that content should probably live in obs_base, or somewhere similar, with this file only noting the specific steps that are different for DECam. I don't know if we should just get this merged now, or figure out a better place for the generic stuff first.

README.md

config/isr.py

doc/lsst.obs.decam/index.rst

doc/lsst.obs.decam/using-obs-decam.rst

parejkoj · 2021-07-09T01:07:58Z

doc/lsst.obs.decam/using-obs-decam.rst

+
+Ingest raw science and calibration (bias and flat) frames, which creates the `DECam/raw/all` collection. Run overscan correction on all the raws so they may be used as crosstalk sources during ISR.
+
+This example assumes the user has setup the `~lsst.cp.pipe`_ module, which is needed for constructing calibration products from raws.


I think any example like this should start with setup lsst_distrib as step 0 (and for the gen2 section below), so that the user has everything necessary?

I don't see setup lsst_distrib anywhere in the doc yet?

These lines are outdated; there's a mention of lsst_distrib now on line 7, which I guess should be 2 lines, because sentences.

parejkoj · 2021-07-09T01:10:59Z

doc/lsst.obs.decam/using-obs-decam.rst

+
+.. prompt:: bash
+
+   mkdir /path/to/repo


For anything with a path like this, it's probably better to have set BUTLER_PATH=/path/to/repo as the first step, and then refer to $BUTLER_PATH everywhere below, so that the user doesn't actually have to do any search and replace.

I don't personally find utilizing environment variables like this very newbie-user-friendly tbh. I saw advice from someone... Tim? K-T? Jim? that working in the butler repo isn't necessarily a good plan, so I will at least change all the dots to REPO. I'm not going to much with the Gen2 stuff.

Responding here (even though this is the gen2 section: yes, I used similar comments):

What is the problem with environment variables? It succinctly solves the the copy/paste problem of paths, and makes scripting tasks like this easier. We're not (or, at least I don't think we are) documenting "how to ingest data into the Rubin Pipeline for a complete commandline-user newbie".

You say we're not doing that, but I'd wager the majority of folks who bother to click through to these docs will be doing exactly that. Maybe I'm unique in saying I don't like environment variables for tutorials. I feel like they add a level of complexity without adding any clarity. Put yourself in the shoes of an undergrad who is learning unix, python, Rubin shenanigans, FITS files, git, all of it for the first time. It's one more thing when there doesn't need to be one more thing.

If they're an undergrad learning Rubin for the first time, they really shouldn't be trying to ingest data, especially right now given that we're still under development.

parejkoj · 2021-07-09T01:12:18Z

doc/lsst.obs.decam/using-obs-decam.rst

+
+.. prompt:: bash
+
+   cd /path/to/desired/repo


For anything with a path like this, it's probably better to have set BUTLER_PATH=/path/to/repo as the first step, and then refer to $BUTLER_PATH everywhere below (instead of . in this example), so that the user isn't running all of the commands directly in the given directory (so that, for example, log files or redirected output don't get written to the butler directory).

Especially for gen3, users should not be touching or writing files in the butler repo itself (since we now have a working S3 backend, for example).

haha, I think you left 2 versions of the same comment twice, or else I just assumed this bit was also what you meant 😀

doc/lsst.obs.decam/using-obs-decam.rst

Remove stub docs for deprecated tasks and add summary docs for existing tasks

config/isr.py

doc/lsst.obs.decam/index.rst

doc/lsst.obs.decam/using-obs-decam.rst

parejkoj · 2021-07-12T23:25:46Z

doc/lsst.obs.decam/using-obs-decam.rst

+   butler query-dimension-records REPO exposure --where "instrument='DECam' AND exposure.observation_type='zero'"
+   butler query-dimension-records REPO exposure --where "instrument='DECam' AND exposure.observation_type='dome flat'"
+
+Use `~lsst.cp.pipe`_ to build nightly calibs


I don't think you can put links into section headers? I'd say Use cp_pipe to build...here and then put the link itself in the intro sentence.

You're no fun. I put the new syntax and left the link in the header. We'll see what happens.

Nope, it doesn't work, even in the full pipelines build. Just make it cp_pipe here, and put the :py:mod: link in the first sentence.

doc/lsst.obs.decam/using-obs-decam.rst

parejkoj · 2021-07-12T23:39:18Z

doc/lsst.obs.decam/using-obs-decam.rst

+
+   When running the ``cpBias.yaml`` pipeline, append ``-c isr:overscan.fitType='MEDIAN_PER_ROW'``.
+
+   When running the ``cpFlat.yaml`` pipeline, append ``-c isr:overscan.fitType='MEDIAN_PER_ROW' -c cpFlatNorm:level='AMP'``.


The way this renders in the built docs makes it a bit hard to see what is to be appended. Can a .. note:: have a .. .. code-block:: none in it?

Similarly for the other place where you have an "append` block like this.

I tried, and the answer is no. I can put the thing to append on its own line, though.

Making them separate lines helped, though a colon after the appends is probably now in order.

parejkoj · 2021-07-12T23:46:17Z

doc/lsst.obs.decam/using-obs-decam.rst

+
+.. prompt:: bash
+
+   mkdir /path/to/repo


Responding here (even though this is the gen2 section: yes, I used similar comments):

What is the problem with environment variables? It succinctly solves the the copy/paste problem of paths, and makes scripting tasks like this easier. We're not (or, at least I don't think we are) documenting "how to ingest data into the Rubin Pipeline for a complete commandline-user newbie".

parejkoj · 2021-07-12T23:51:29Z

doc/lsst.obs.decam/using-obs-decam.rst

+.. note::
+   Until `DM-30651 <https://jira.lsstcorp.org/browse/DM-30651>`__ is resolved, consider configuring ``-c isr:overscan.fitType='MEDIAN_PER_ROW'`` as above when running ISR.
+
+Some useful pipelines can be found in the ``pipelines/DarkEnergyCamera`` directory of the `~lsst.ap.pipe`_ package.


I think this needs to be:

:py:mod:`lsst.ap.pipe`

to get an actual link there. My package-docs build is failing right now (I think I'm out of date on some things), so I can't check.

Oh is that the new way to do it? I thought that only worked for "normal" python modules and not our own, as the rst guide only has very generic examples. But I think you're right. The failure state is bold text in lieu of a link, which looks better than squiggle thingy at any rate.

doc/lsst.obs.decam/using-obs-decam.rst

parejkoj · 2021-07-12T23:55:29Z

doc/lsst.obs.decam/using-obs-decam.rst

+
+Ingest raw science and calibration (bias and flat) frames, which creates the `DECam/raw/all` collection. Run overscan correction on all the raws so they may be used as crosstalk sources during ISR.
+
+This example assumes the user has setup the `~lsst.cp.pipe`_ module, which is needed for constructing calibration products from raws.


I don't see setup lsst_distrib anywhere in the doc yet?

mrawls · 2021-07-13T00:45:49Z

doc/lsst.obs.decam/using-obs-decam.rst

+The Gen3 workflow
+#################
+
+These directions are for users who wish to create their own data repositories independent of any Rubin/LSST-supported shared repositories. They assume the user has a recent weekly version of ``lsst_distrib`` setup.


This is where I put the mention about lsst_distrib

This long-overdue change brings obs_decam closer in line with Gen3 standards. The old default 'cp' prefix refers to the DECam Community Pipeline, but the Gen3 workflow uses our own cp_pipe to build 'bias' and 'flat' data products instead of ingesting third-party calibs.

parejkoj

There are a couple more comments above to clean up the section header non-link and the notes boxes, otherwise this looks good. I built the full pipelines docs and it looks nice.

Thanks for cleaning this up. I do hope we can get together a group of people who have written this kind of doc and get it all collated into one central location as part of the main pipelines docs.

mrawls force-pushed the tickets/DM-24913 branch from 78c46c1 to 5252db0 Compare July 3, 2021 01:06

mrawls requested a review from parejkoj July 3, 2021 01:06

parejkoj requested changes Jul 9, 2021

View reviewed changes

mrawls force-pushed the tickets/DM-24913 branch from 5252db0 to 048d079 Compare July 10, 2021 05:56

mrawls added 2 commits July 10, 2021 01:44

Update stub task documentation

45611e9

Remove stub docs for deprecated tasks and add summary docs for existing tasks

Remove old example astrometry script

05bfd6b

mrawls force-pushed the tickets/DM-24913 branch from 048d079 to 19e5e15 Compare July 10, 2021 06:45

parejkoj reviewed Jul 12, 2021

View reviewed changes

mrawls commented Jul 13, 2021

View reviewed changes

mrawls force-pushed the tickets/DM-24913 branch from 19e5e15 to 5b5eb78 Compare July 13, 2021 01:13

parejkoj approved these changes Jul 14, 2021

View reviewed changes

mrawls added 3 commits July 22, 2021 18:27

Add brief DECam user guide

21c9cc8

Minimal README now points to pipelines documentation

18cbcf9

Update doc/conf.py for documenteer 0.6

f080263

mrawls force-pushed the tickets/DM-24913 branch from 5b5eb78 to f080263 Compare July 22, 2021 23:27

mrawls merged commit 873942f into master Jul 22, 2021

mrawls deleted the tickets/DM-24913 branch July 22, 2021 23:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-24913: obs_decam README is confusing #196

DM-24913: obs_decam README is confusing #196

mrawls commented Jul 3, 2021

parejkoj left a comment

parejkoj Jul 9, 2021

parejkoj Jul 12, 2021

mrawls Jul 13, 2021

parejkoj Jul 9, 2021

mrawls Jul 10, 2021

parejkoj Jul 12, 2021

mrawls Jul 13, 2021

parejkoj Jul 13, 2021 •

edited

parejkoj Jul 9, 2021

mrawls Jul 10, 2021

parejkoj Jul 12, 2021

mrawls Jul 13, 2021

parejkoj Jul 14, 2021

parejkoj Jul 12, 2021 •

edited

mrawls Jul 13, 2021

parejkoj Jul 14, 2021

parejkoj Jul 12, 2021

parejkoj Jul 12, 2021

mrawls Jul 13, 2021

parejkoj Jul 12, 2021

mrawls Jul 13, 2021

parejkoj left a comment


		Ingest raw science and calibration (bias and flat) frames, which creates the `DECam/raw/all` collection. Run overscan correction on all the raws so they may be used as crosstalk sources during ISR.

		This example assumes the user has setup the `~lsst.cp.pipe`_ module, which is needed for constructing calibration products from raws.


		When running the ``cpBias.yaml`` pipeline, append ``-c isr:overscan.fitType='MEDIAN_PER_ROW'``.

		When running the ``cpFlat.yaml`` pipeline, append ``-c isr:overscan.fitType='MEDIAN_PER_ROW' -c cpFlatNorm:level='AMP'``.

DM-24913: obs_decam README is confusing #196

DM-24913: obs_decam README is confusing #196

Conversation

mrawls commented Jul 3, 2021

parejkoj left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parejkoj Jul 13, 2021 • 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

parejkoj Jul 12, 2021 • 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

Choose a reason for hiding this comment

parejkoj left a comment

Choose a reason for hiding this comment

parejkoj Jul 13, 2021 •

edited

parejkoj Jul 12, 2021 •

edited