DM-44955: Update eff_time metric documentation #231
Conversation
| @@ -1227,23 +1227,20 @@ tables: | |||
| - name: eff_time | |||
| "@id": "#ccdvisit1_quicklook.eff_time" | |||
| datatype: float | |||
| description: Effective exposure time. | |||
| description: Effective exposure time calculated from PSF sigma, sky background, and zero point. | |||
| ivoa:unit: s | |||
| - name: eff_time_psf_sigma_scale | |||
| "@id": "#ccdvisit1_quicklook.eff_time_psf_sigma_scale" | |||
| datatype: float | |||
| description: Effective exposure time, PSF sigma scale. | |||
There was a problem hiding this comment.
It might make sense to change the description to be "Scale factor for effective exposure time based on PSF sigma".
There was a problem hiding this comment.
100%. Also perhaps a better name pattern would be eff_time_scale_psf_sigma (etc.) -- it's an "effective time scale [factor]" due to "PDF sigma"
There was a problem hiding this comment.
If you are interested in the original naming discussion surrounding these quantities...
https://lsstc.slack.com/archives/C01JTSDV7CH/p1706307463383399
| - name: eff_time_psf_sky_bg_scale | ||
| "@id": "#ccdvisit1_quicklook.eff_time_sky_bg_scale" | ||
| datatype: float | ||
| description: Effective exposure time, sky backgroundscale. | ||
| ivoa:unit: s | ||
| description: Effective exposure time, sky background scale. |
| @@ -1227,23 +1227,20 @@ tables: | |||
| - name: eff_time | |||
| "@id": "#ccdvisit1_quicklook.eff_time" | |||
| datatype: float | |||
| description: Effective exposure time. | |||
| description: Effective exposure time calculated from PSF sigma, sky background, and zero point. | |||
| ivoa:unit: s | |||
| - name: eff_time_psf_sigma_scale | |||
| "@id": "#ccdvisit1_quicklook.eff_time_psf_sigma_scale" | |||
| datatype: float | |||
| description: Effective exposure time, PSF sigma scale. | |||
| - name: eff_time_psf_sky_bg_scale | ||
| "@id": "#ccdvisit1_quicklook.eff_time_sky_bg_scale" | ||
| datatype: float | ||
| description: Effective exposure time, sky backgroundscale. | ||
| ivoa:unit: s | ||
| description: Effective exposure time, sky background scale. |
| - name: eff_time_psf_sky_bg_scale | ||
| "@id": "#ccdvisit1_quicklook.eff_time_sky_bg_scale" | ||
| datatype: float | ||
| description: Effective exposure time, sky backgroundscale. | ||
| ivoa:unit: s | ||
| description: Effective exposure time, sky background scale. | ||
| - name: eff_time_psf_zero_point_scale | ||
| "@id": "#ccdvisit1_quicklook.eff_time_zero_point_scale" | ||
| datatype: float | ||
| description: Effective exposure time, zero point scale. |
|
These are unitless, normalized scale factors (as indicated by the |
|
If/when UCDs are added to these tables, |
I don't believe those changes were actually made. |
|
Sorry, I didn't see that comment. What is the procedure for reopening a ticket? (As an aside, I thought that it was almost definitionally implied that scale factors were dimensionless.) |
|
Not being familiar with what was being done algorithmically, the implied grammar of the word order "Effective exposure time, sky background scale" did not clearly convey "scale factor based on contribution X", at least to me. The group of these read more to me like implying something like "effective exposure time, algorithm version X". I get it now, now that you've explained it, but we'll have a lot of newbies reading these columns names and descriptions. There's plenty of room in these description strings, so you can just spell out "scale factor". I think @ktlim's suggestion is good. My suggested column name ordering change also, I think, strengthens the idea that we're looking at several contributions that are parallel to each other, and they'll group more clearly in lists of columns. |
|
About your "how to reopen": https://developer.lsst.io/work/flow.html#workflow-code-review doesn't seem to address this point, and searching on developer.lsst.io for "reopen" doesn't help either. I think @ktlim or @timj would be more likely to have a project-traditions-based answer for you on that point than I. |
|
Changing the column names is a somewhat bigger deal. It changes the interface, requiring synchronization with the publishing code in Rapid Analysis. (It also requires a manual edit of the migration script, but at least that's a one-time thing.) So let's make sure that we can't live with what we have, and there's no way I'd allow that to merge before OR4. You cannot reopen a ticket branch. (You can reopen a ticket, but that's probably only useful if there's a different package that needs a change as well, or if the ticket is not accomplished via code change.) You can add a hotfix if something is broken, but in this case, the best thing is to just create a new one, even if it's nearly-trivial. |
No worries there; I am not up to speed on how much of what's in the schema is already being actively written.
🤷 The descriptions will help a lot. I recognize that we won't be able to polish away all the sharp edges. |
|
Ok, I've opened a new ticket (DM-44958). The original documentation for the eff_time quantities is here: I think that the descriptions here were written without knowledge of that documentation. I will update the descriptions here to be more consistent with the original documentation (and maybe better than the original descriptions). Regarding name changes, I think it is nice to have the names here match what is in exposureSummaryStats. There was a reasonable amount of discussion that converged on these names (I won't claim that discussion converged on the global minimum, just that we went around several loops before arriving here...). |
gpdf
changed the title
Very small amount of clean up for eff_time-related entries.