Details
-
Type:
Story
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: meas_mosaic
-
Labels:None
-
Story Points:1
-
Epic Link:
-
Sprint:AP F18-5
-
Team:Alert Production
Description
With w_2018_41, meas_mosaic fails with errors like
root WARN: Failed to read {'ccd': 24, 'visit': 1324, 'tract': 9615, 'filter': 'HSC-I'}: "Field with name 'i_instFlux' not found"
|
and then Segmentation fault from zero matchList (Mosaic INFO: len(matchList) = 0 [] )
One (longer than necessary) command to reproduce is
mosaic.py /datasets/hsc/repo --calib /datasets/hsc/repo/CALIB --rerun RC/w_2018_41/DM-16011:private/usename/ --numCoresForReadSource=12 --id ccd=0..8^10..103 visit=26024^26028^26032^26036^26044^26046^26048^26050^26058^26060^26062^26070^26072^26074^26080^26084^26094 tract=9615
|
The same command (with the same input data) works using the w_2018_39 stack.
Attachments
Issue Links
- relates to
-
- Done
Activity
Hsin-Fang Chiang
created issue -
Hsin-Fang Chiang
made changes -
Hsin-Fang Chiang
made changes -
| Risk Score | 0 |
A shorter/faster command to reproduce:
mosaic.py /datasets/hsc/repo --calib /datasets/hsc/repo/CALIB --rerun RC/w_2018_41/DM-16011:private/user/name --id visit=380^384 filter=HSC-Y tract=9615
|
Hsin-Fang Chiang
added a comment - A shorter/faster command to reproduce:
mosaic.py /datasets/hsc/repo --calib /datasets/hsc/repo/CALIB --rerun RC/w_2018_41/DM-16011:private/user/name --id visit=380^384 filter=HSC-Y tract=9615
| Assignee | John Parejko [ parejkoj ] |
More of these? I thought I caught them all.
DM-15837 didn't fix this?
DM-15837 fixed it that w_2018_39 runs fine. But somehow it got broken again and w_2018_41 fails.
Hsin-Fang Chiang
added a comment - DM-15837 fixed it that w_2018_39 runs fine. But somehow it got broken again and w_2018_41 fails. I just ran your short command above with meas_mosaic master and w_2018_41 everything else, and it ran to completion with no errors.
Weird. I tried again with the short command with w_2018_41 (which should be the same as master for meas_mosaic) and it failed again.
Hsin-Fang Chiang
added a comment - Weird. I tried again with the short command with w_2018_41 (which should be the same as master for meas_mosaic) and it failed again. Ah-hah! I hadn't updated my master to actually be master. Error confirmed: I'll dig into it.
Ok, here's a PR that should fix it. It looks like the fixes that Jim Bosch made to how we deal with flux/instFlux in old catalogs made my fix in DM-15837 obsolete.
https://github.com/lsst/meas_mosaic/pull/45
That said, I can't be sure that this doesn't break meas_mosaic master on pre-existing catalogs.
| Reviewers | Hsin-Fang Chiang [ hchiang2 ] | |
| Status | To Do [ 10001 ] | In Review [ 10004 ] |
I confirm that the fix + w_2018_41 stack works fine to finish the above test command.
I'm thinking Jim Bosch might be a better reviewer to see if this is all consistent with other part of the stack and other related tickets, so I'm changing the reviewer to him.
Hsin-Fang Chiang
added a comment - I confirm that the fix + w_2018_41 stack works fine to finish the above test command.
I'm thinking Jim Bosch might be a better reviewer to see if this is all consistent with other part of the stack and other related tickets, so I'm changing the reviewer to him. Hsin-Fang Chiang
made changes -
| Reviewers | Hsin-Fang Chiang [ hchiang2 ] | Jim Bosch [ jbosch ] |
Looks fine. Does anyone here know of anywhere else that would require a similar fix? (i.e. using "instflux" on non-instrumental-flux fields as a workaround)
| Status | In Review [ 10004 ] | Reviewed [ 10101 ] |
pipe_analysis came to mind immediately, but Lauren MacArthur might already be working on it.
Hsin-Fang Chiang
added a comment - pipe_analysis came to mind immediately, but Lauren MacArthur might already be working on it. Is this really a workaround, or just reflective of the fact that reference catalogs don't have instFlux entries?
This is reverting the fix that was necessary because refcats were gaining an instFlux before Jim Bosch fixed the aliasing logic.
Thanks for the quick review: merged and done.
| Resolution | Done [ 10000 ] | |
| Status | Reviewed [ 10101 ] | Done [ 10002 ] |
To further clarify for posterity:
- Reference catalogs were incorrectly being read in with instFlux fields following the initial rename of flux to instFlux in source catalogs.
- That was fixed in afw in
DM-15857, but not beforeDM-15837added a workaround to meas_mosaic to make it expect instFlux fields in reference catalogs. The afw fix broke the meas_mosaic workaround; my question above was whetherDM-15837or any other ticket added any other workarounds that will have been broken by the more correct fix in afw. DM-15891also addressed some fallout from the instFlux change, but was always compatible with the afw fix inDM-15857.DM-16068augmented the afw fix fromDM-15857to handle some cases it had originally missed.- This ticket (
DM-16170) reverts theDM-15837workaround.
That said, I can't be sure that this doesn't break meas_mosaic master on pre-existing catalogs.
I'll confirm this on DM-15916. UPDATE: confirmed with that branch 
| Epic Link |
|
| Sprint | AP F18-5 [ 750 ] | |
| Story Points | 1 | |
| Team | Alert Production [ 10300 ] |
Hey John Parejko — are you able to take a look at this? I think for this ticket it's fine to solve the field naming issue; the fact that meas_mosaic segfaults is out of scope.