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-30895: Move AssembleCoaddTask and its children from pipe_tasks #26

Merged
merged 12 commits into from Sep 20, 2023

Conversation

arunkannawadi
Copy link
Member

No description provided.

@arunkannawadi arunkannawadi changed the title DM-30895: Move AssembleCoaddTask and its children to drp_tasks DM-30895: Move AssembleCoaddTask and its children from pipe_tasks Sep 13, 2023
Copy link
Contributor

@fred3m fred3m left a comment

Choose a reason for hiding this comment

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

Just a few small comments and corrections.

self.doUsePsfMatchedPolygons = True

# Real EDGE removed by psfMatched NO_DATA border half the width of the
# matching kernel CompareWarp applies psfMatched EDGE pixels to
Copy link
Contributor

Choose a reason for hiding this comment

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

insert period after "kernel".


Parameters
----------
spanSetList : `list` [`lsst.afw.geom.SpanSet`]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that this should match the type hints syntax list[lsst.afw.geom.SpanSet]. Same with all other docstrings in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

The above is correct. Sphinx can't parse type annotations so you have to backtick quote each type separately.

Comment on lines 1060 to 1099
@staticmethod
def _subBBoxIter(bbox, subregionSize):
"""Iterate over subregions of a bbox.

Parameters
----------
bbox : `lsst.geom.Box2I`
Bounding box over which to iterate.
subregionSize : `lsst.geom.Extent2I`
Size of sub-bboxes.

Yields
------
subBBox : `lsst.geom.Box2I`
Next sub-bounding box of size ``subregionSize`` or smaller; each
``subBBox`` is contained within ``bbox``, so it may be smaller than
``subregionSize`` at the edges of ``bbox``, but it will never be
empty.

Raises
------
RuntimeError
Raised if any of the following occur:
- The given bbox is empty.
- The subregionSize is 0.
"""
if bbox.isEmpty():
raise RuntimeError("bbox %s is empty" % (bbox,))
if subregionSize[0] < 1 or subregionSize[1] < 1:
raise RuntimeError("subregionSize %s must be nonzero" % (subregionSize,))

for rowShift in range(0, bbox.getHeight(), subregionSize[1]):
for colShift in range(0, bbox.getWidth(), subregionSize[0]):
subBBox = geom.Box2I(bbox.getMin() + geom.Extent2I(colShift, rowShift), subregionSize)
subBBox.clip(bbox)
if subBBox.isEmpty():
raise RuntimeError("Bug: empty bbox! bbox=%s, subregionSize=%s, "
"colShift=%s, rowShift=%s" %
(bbox, subregionSize, colShift, rowShift))
yield subBBox
Copy link
Contributor

Choose a reason for hiding this comment

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

This is in lsst.pipe.tasks.coaddBase, so it can be removed as a static method of this class.

@arunkannawadi
Copy link
Member Author

arunkannawadi commented Sep 19, 2023

Great catches, Fred! I've addressed the comments now.

  • Move assembleChi2Coadd.py
  • Remove the subBBoxIter static method (and use the one from coaddBase)
  • Add a period of kernel in a docstring

File(s) transferred:

  - python/lsst/pipe/tasks/assembleCoadd.py ->
    python/lsst/pipe/tasks/assemble_coadd.py
File(s) transferred:

  - python/lsst/pipe/tasks/dcrAssembleCoadd.py ->
    python/lsst/pipe/tasks/dcr_assemble_coadd.py
Files transferred:
- python/lsst/pipe/tasks/assembleChi2Coadd.py ->
  python/lsst/drp/tasks/assemble_chi2_coadd.py
File(s) transferred:

  - tests/assembleCoaddUtils.py ->
    tests/_assemble_coadd_utils.py
File(s) transferred:

  - tests/test_assembleCoadd.py ->
    tests/test_assemble_coadd.py
  - tests/test_dcrAssembleCoadd.py ->
    tests/test_dcr_assemble_coadd.py
@arunkannawadi arunkannawadi merged commit 4394590 into main Sep 20, 2023
2 checks passed
@arunkannawadi arunkannawadi deleted the tickets/DM-30895 branch September 20, 2023 04:01
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

3 participants