DM-36571: Remove applyColorTerms=None option from PhotoCalTask and default to False #727
Conversation
python/lsst/pipe/tasks/photoCal.py
Outdated
| default=False, | ||
| doc=("Apply photometric color terms to reference stars?\n" | ||
| "True: always apply colorterms; fail if color term data is not available for the " | ||
| "specified reference catalog and filter.\n" |
There was a problem hiding this comment.
This doc string line would be 120 characters long. Would it look nicer to maintain the indented look (indenting to match True: above) as before?
There was a problem hiding this comment.
This spawned a whole conversation in #dm-docs-support. The challenge is balancing the config.save output vs. the rendered HTML that goes in pipelines.lsst.io. I think I've found a good balance of those here.
https://lsstc.slack.com/archives/C2B6DQBAL/p1667327052273339
There was a problem hiding this comment.
Had you previously removed the always in the True section here? If so, it looks like it has reappeared.
I still think it would read better if written as attempt to apply color terms; fail if.... If instead you simply write apply color terms; fail if..., that implies that you a) are applying color terms, but then b) are failing to apply color terms, because none are available. Logically, this doesn't make sense to me, hence the need for an admission of an attempt being made.
Follow up point: use of colorterms vs color terms. I think the former should be reserved for reference to the config parameter, whilst the latter reserved for discussion of "color terms". I think this sentence probably should make use of color terms (with a space), but happy to defer to you if you were instead meaning to refer to the config parameter.
There was a problem hiding this comment.
Ok, I think I see what you mean. I've taken that wording.
python/lsst/pipe/tasks/photoCal.py
Outdated
| "False: do not apply."), | ||
| default=False, | ||
| doc=("Apply photometric color terms to reference stars?\n" | ||
| "True: always apply colorterms; fail if color term data is not available for the " |
There was a problem hiding this comment.
Perhaps this would be clearer if written:
True: attempt to apply color terms; fail if color term data...
There was a problem hiding this comment.
I've taken out the "always"; the "attempt" is implied by the failure description.
There was a problem hiding this comment.
Thanks - as mentioned in a reply above, I think your edit was undone by a subsequent edit. For the reasons elucidated above, I still prefer the use of attempt to here, but am happy to defer to your judgement on this.
parejkoj
force-pushed
the
tickets/DM-36571
branch
2 times, most recently
from
2a1ee48
to
014b2b0
Compare
|
@leeskelvin : what do you think about this reworked version? I think the rendered HTML and |
|
Thanks John, this is looking good. I left a comment on the new doc string format, but otherwise I think this looks good to me, cheers! |
This option did not behave the way some expected, and could result in colorterms not being applied when one was expecting them to be.
parejkoj
force-pushed
the
tickets/DM-36571
branch
from
014b2b0
to
40034b3
Compare
No description provided.