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-28717: Allow for Stamps formatter to handle bbox #573

Merged
merged 3 commits into from Mar 10, 2021

Conversation

MorganSchmitz
Copy link
Contributor

One of several PRs for DM-28717, the others being in daf_butler, meas_algorithms and obs_base.

if options.exists("llcX"):
if "bbox" in options.keys():
bbox = options["bbox"]
elif isinstance(options, PropertySet) and options.exists("llcX"):
Copy link
Member

Choose a reason for hiding this comment

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

This should support dict and not be restricted to PropertySet. You can use in so this will work just like the check for bbox above:

elif "llcx" in options:

@@ -52,6 +53,9 @@ def imageReadFitsWithOptions(cls, source, options):
- imageOrigin: one of "LOCAL" or "PARENT" (has no effect unless
a bbox is specified by llcX, etc.)

Alternatively, a bounding box () can be passed on with the
``"bbox"'' key.
Copy link
Member

Choose a reason for hiding this comment

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

You should document what type the value should be.

origin = ImageOrigin.PARENT
else:
raise RuntimeError("Unknown ImageOrigin type {}".format(originStr))
if options.exists("imageOrigin"):
Copy link
Member

Choose a reason for hiding this comment

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

Change this to be dict compatible as if "imageOrigin" in options

@@ -25,6 +25,7 @@
import lsst.geom
from lsst.log import Log
from lsst.afw.fits import ImageWriteOptions
from lsst.daf.base import PropertySet
Copy link
Member

Choose a reason for hiding this comment

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

This line is not needed if you use the dict compatibility interface of PropertySet

@MorganSchmitz MorganSchmitz force-pushed the tickets/DM-28717 branch 2 times, most recently from f098e33 to f9232ea Compare March 4, 2021 18:39
So the correct exception is caught when options are passed on as a dict
instead of a PropertySet. Also ensure subTest receives a str.
@MorganSchmitz MorganSchmitz merged commit 5d80ea2 into master Mar 10, 2021
@MorganSchmitz MorganSchmitz deleted the tickets/DM-28717 branch March 10, 2021 13:31
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