Skip to content

DM-44656: Enable PeekExposureTask for LSSTComCam#109

Merged
jmeyers314 merged 2 commits intomainfrom
tickets/DM-44656
Jun 4, 2024
Merged

DM-44656: Enable PeekExposureTask for LSSTComCam#109
jmeyers314 merged 2 commits intomainfrom
tickets/DM-44656

Conversation

@jmeyers314
Copy link
Copy Markdown
Contributor

No description provided.

Comment on lines +974 to +979
try:
isDisp = isDispersedExp(exposure)
except Exception as e:
self.log.warning(f"isDispersedExp failed: {e}. Assuming not dispersed")
isDisp = False
if isDisp:
mode = "spec"
else:
mode = "photo"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There's two things I don't love here: exceptions as control flow (not so bad given this won't be in a tight loop), and the fact this will routinely spew really quite scary looking warnings (made somewhat worse still because it includes the whole "{e}" in the message) during normal/expected operation (which is really bad behaviour IMO). I think it shouldn't be too hard to avoid that though. I've just pushed a commit (untested) that I think should give you what you want though. Could you test that it does what you want? If it does, then that's a 👍 from me (obviously).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, that looks good to me. I've reordered some of the if/else logic and removed the unnecessary isDisp variable, but followed your same logic.

@jmeyers314 jmeyers314 merged commit b61a23c into main Jun 4, 2024
@jmeyers314 jmeyers314 deleted the tickets/DM-44656 branch June 4, 2024 21:21
Comment on lines +974 to +979
if exposure.getInfo().getVisitInfo().instrumentLabel == "LATISS":
# only LATISS images *can* be dispersed, and isDispersedExp
# only works cleanly for LATISS
mode = "spec" if isDispersedExp(exposure) else "photo"
else:
mode = "photo"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FWIW, and now we're probably just in the land of personal preference, but I'd write that as:

mode = "photo"
if exposure.getInfo().getVisitInfo().instrumentLabel == "LATISS":
    # only LATISS images *can* be dispersed, and isDispersedExp
    # only works cleanly for LATISS
    mode = "spec" if isDispersedExp(exposure)

because otherwise you end up with a variable that's defined inside if/else blocks which could end up, if the logic changed in the future, undefined, and this way you'll always have the fallback. Definitely in the noise though 🙂

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh, except that's a supplied arg... anyway, whatever, all is good, this looks great 🙂 👍

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.

2 participants