DM-54194: Add second moment in sky /AltAz and fgcm phot for PSF single visit table#1263
DM-54194: Add second moment in sky /AltAz and fgcm phot for PSF single visit table#1263
Conversation
kklaliotis
left a comment
kklaliotis
left a comment
There was a problem hiding this comment.
I double-checked the conversions here and they all agreed with what I was doing before.
I think the only change I'd maybe make is making the new column names align a little better with the current versions of the catalog columns. For instance, the "I" has mostly been removed so source moments are called slot_Shape_xx now. So then I would maybe include the sky coords moments as slot_Shape_uu. The alt-az labels are a bit clunky but I think for the sake of clarity I'd leave them essentially as they are, just again omitting the I to be eg. slot_Shape_altalt . Similarly I'd shift the naming convention for psf shape columns to match the others with PsfShape now instead of psfShape .
But I think overall the actual implementation looks good!
rmjarvis
left a comment
rmjarvis
left a comment
There was a problem hiding this comment.
I don't thnk the formulae are quite right.
|
Maybe also useful to look at the unit test we have in GalSim that validates that code: |
|
@PFLeget - is this something that you're looking to backport to v30? |
|
Another point of concern here -- I suspect the sign of Iuv is wrong. Or at least contrary to the standard WL definition. We typically define +v to be towards North and +u to be towards West. I.e. normal axis orientation when looking at the galaxy from Earth if North is up. The standard CD matrix in the WCS specs puts +u to the East. I.e. towards increasing RA, rather than decreasing. It would be convenient if this routine converted the moments into the normal WL convention, rather than requiring an addition sign flip to g2 downstream of this function. |
Would it be just as good from your perspective if we applied that flip between the second moments and (g1, g2)? I'm a little wary of changing our sky axis definition for moments to be something other than the WCS definition, since those have non-lensing consumers. |
Up to you I guess. Personally, I find the WCS convention weird. The u,v coordinates are from the perspective of God looking through the universe down at Earth. But whatever you find appropriate I suppose. |
|
@arunkannawadi I would love to, but I am not sure if this is ok. If I can add those column like that this is available for everyone latter on, it would be great for PSF tests. I will make a run anyway at least for me on DP2 to have access and move forward in digging in DP2. |
PFLeget
force-pushed
the
tickets/DM-54194
branch
2 times, most recently
from
e079262 to
94e5a1c
Compare
| if not self.config.psf_determiner['piff'].useColor: | ||
| # Keep fgcm_standard_star if using chromatic PSF OR if adding FGCM photometry | ||
| needs_fgcm = (self.config.psf_determiner['piff'].useColor | ||
| or self.config.doAddFgcmPhotometry) |
There was a problem hiding this comment.
From what I can see, super().__init__ is a no-op and doesn't make config an attribute. Replace self.config with config.
There was a problem hiding this comment.
But this is working as is now. Shall I change it?
| ivv[i] *= deg2_to_arcsec2 | ||
| iuv[i] *= deg2_to_arcsec2 | ||
|
|
||
| return iuu, ivv, iuv |
There was a problem hiding this comment.
Given the confusion we've had, it would be useful to have this in a unit test, similar to the one that Colin had in #1273 .
| doAddFgcmPhotometry = pexConfig.Field( | ||
| dtype=bool, | ||
| default=True, | ||
| doc="Add FGCM photometry for all bands (u,g,r,i,z,y) to the output table.", |
There was a problem hiding this comment.
Remove the list of bands from here.
PFLeget
force-pushed
the
tickets/DM-54194
branch
2 times, most recently
from
67aa170 to
58171a6
Compare
PFLeget
force-pushed
the
tickets/DM-54194
branch
from
b86a42a to
0193385
Compare
7 participants
Adding optional information to write in PSF table for analysis.