Details
-
Type:
Story
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: afw, meas_algorithms
-
Labels:None
-
Story Points:8
-
Team:Data Release Production
Description
We need to create polygon-based masks of the usable area of the focal plane, persist them with exposure, and include them in coaddition of PSFs and aperture corrections.
This includes HSC issues HSC-972, HSC-973, HSC-974, HSC-975, HSC-976.
At least some of this will be blocked by DM-833, which is the port issue for coaddition of aperture corrections.
Attachments
Issue Links
Activity
| Labels | HSC |
| Component/s | afw [ 10714 ] | |
| Component/s | meas_algorithms [ 10732 ] |
We don't actually have any text-based format for the polygons right now; they're created programmatically and persisted to FITS binary tables.
| Assignee | John Swinbank [ swinbank ] |
| Assignee | John Swinbank [ swinbank ] |
| Sprint | Science Pipelines DM-S15-5 [ 162 ] |
| Sprint | Science Pipelines DM-S15-5 [ 162 ] |
| Assignee | John Swinbank [ swinbank ] | Russell Owen [ rowen ] |
Assigning this to Russell following discussion with Simon Krughoff on 26 June.
| Link | This issue is contained by DLP-287 [ DLP-287 ] |
| Link | This issue is contained by DLP-287 [ DLP-287 ] |
It looks like the first step is to pull over the use of Polygon in Exposure and ExposureInfo.
Yes, that's right. We already have an older version of the Polygon class on the LSST side, actually, but it's directly in afw::geom, and on the HSC side I found that I had to move it to a subpackage in order to avoid some Swig circular dependencies when adding persistence to it.
Paul: do you have time to look at this? The code itself will not need much review as it is cherry-picked from HSC. What needs review is any changes I made and my commits. All work is on tickets/DM-2981 of afw.
I imagine it will be obvious, but the sequence of commits is as follows:
- The first six commits are directly cherry-picked from HSC, followed by a few of mine that modernize the code. That set introduces the Polygon class.
- The next cherry-pick adds validPolygon to ExposureInfo, followed by one trivial cleanup commit on my part.
- The final commit fixes
DM-3214, a bug that was exposed by an existing unit test but that was triggered by adding validPolygon to ExposureINfo. The fix was so trivial that I was given permission to fix it here. So I intend to take permission to merge this as permission to closeDM-3214as well.
| Reviewers | Paul Price [ price ] | |
| Status | To Do [ 10001 ] | In Review [ 10004 ] |
Looks good. I have just a few minor suggestions that you may or may not want to update:
- When you have a conflict recorded in the commit message, it's helpful to explain what caused the conflicts and how they were resolved.
- When you say "Modernize ...", could you explain in the commit message what has changed between ancient and modern times?
- For "Modernize Polygon.cc's handling of Point fields in afw tables", the added code has trailing whitespace.
- For "Fix ChebyshevBoundedField's...", your commit message is a bit wide on the last line.
Paul Price
added a comment - Looks good. I have just a few minor suggestions that you may or may not want to update:
When you have a conflict recorded in the commit message, it's helpful to explain what caused the conflicts and how they were resolved.
When you say "Modernize ...", could you explain in the commit message what has changed between ancient and modern times?
For "Modernize Polygon.cc's handling of Point fields in afw tables", the added code has trailing whitespace.
For "Fix ChebyshevBoundedField's...", your commit message is a bit wide on the last line.
Paul Price
made changes -
| Status | In Review [ 10004 ] | Reviewed [ 10101 ] |
Thank you, Paul. I did not remember what the merge conflicts were about, so I could not enhance those merge messages, but I clarified the other messages you mentioned and removed all trailing whitespace from several of the files that were part of this ticket. After a successful Jenkins build I merged to master.
| Resolution | Done [ 10000 ] | |
| Status | Reviewed [ 10101 ] | Done [ 10002 ] |
Note that (contrary to the description) this did not pull over HSC-974. That will be done in DM-3243.
On further digging: HSC-975 doesn't apply until we have CoaddBoundedField (or equivalent), which is DM-833. I'll add a note to that issue to indicate that it's required.
I think we can bring HSC-976 over straight away, though.
| Labels | HSC |
How are you specifying the polygons? STC-S format would be handy.