DM-33957: Make red galaxy truth residual and pull plots #18
Conversation
erykoff
force-pushed
the
tickets/DM-33957
branch
from
a362d6d
to
6f77ca7
Compare
|
Some of the commits can (may be should) be squashed. For e.g, the commit about converting astropy units to numpy arrays also fixes a typo bug. |
| color2_flux2 = df[self.color2_flux2].values*u.Unit(self.color2_flux2_units) | ||
| color2_mag2 = color2_flux2.to(u.ABmag).value | ||
|
|
||
| color1 = color1_mag1 - color1_mag2 |
There was a problem hiding this comment.
This is fine for now, but we should consider this as a technical debt and refactor it later to have a functor/action to calculate colors and use the DifferenceColumns action to compute color difference. I also didn't look at it in too much detail, but wonder if these can be achieved through the data frame actions here.
There was a problem hiding this comment.
I'm curious why using a composite action might be considered better here than a simple subtraction - I am very new to actually using these, so there may be something important I am not aware of, but I worry that this is a case where maximizing use of something we wrote is automatically assumed to be good.
I think the actual criteria for using ConfigurableAction composition instead of writing a non-composite action should be something like this:
- Would there actually be any use cases for swapping out the nested action?
- If so, is it also important for those other use cases to share the parent action from a correctness standpoint (i.e. if we had two non-composite actions instead, and they got out of sync, is that bug)?
For cases where it's just that the nested action is complex enough that we want to encourage it to be reused, I think I'd rather see that complexity moved to other normal classes or functions that could be reused instead.
I'm thinking we might want to raise this at a team meeting; maybe try to brainstorm some conventions and best practices and get them written down.
There was a problem hiding this comment.
My thought was that computing a color given a pair of bands would be a common thing to do, and hence we should have an action that would take in "g" and "r" say and give colors. I am okay with either subtracting the color after that to compute ColorDiff or use DifferenceColumn action.
There was a problem hiding this comment.
I think that this is definitely touching on something philosophical that we should discuss in team meeting. I am not a fan of the nested actions, actually. I much prefer programming in python than trying to turn yaml/pexConfig into a programming language.
The way that the plotting tools can be reused and configured to do different actions for each column is great. But I don't see the problem in each column type being a particular block of python code.
There was a problem hiding this comment.
Yes, topic for a team meeting for sure!
| self.color1_flux1_err, | ||
| self.color1_flux2_err) | ||
|
|
||
| if self.color2_flux1_err and self.color2_flux2_err: |
There was a problem hiding this comment.
I don't quite follow why the errors for color2 have to be treated separately this way.
There was a problem hiding this comment.
In this case, we're subtracting an observed color (with errors) from a true color (without errors). I can foresee a future where we want to subtract an observed color from a separate observed color (both with errors). But if you try to subtract (say) a true color from a true color, both without errors, then you divide by 0 and it won't work.
If you prefer I could treat the color1 errors like this as well, and then just raise in the config validation if you configure such that neither color1 nor color2 has errors.
There was a problem hiding this comment.
I think that'd be good. Alternatively, a comment along this line would also be sufficient.
| color2_mag2_err = 0.0 | ||
|
|
||
| color1 = color1_mag1 - color1_mag2 | ||
| err1_sq = color1_mag1_err**2. + color1_mag2_err**2. |
There was a problem hiding this comment.
You could consider np.hypot of all four columns instead of squaring and taking sqrt later.
There was a problem hiding this comment.
Upon further review, np.hypot only works on two arrays/values, so it's back to square (root) one.
There was a problem hiding this comment.
Fair enough. All looks good to me.
| input_ref_dict = butlerQC.get(inputRefs) | ||
|
|
||
| # Determine columns to read | ||
| obj_columns = set({'objectId', 'patch', 'coord_ra', 'coord_dec'}) |
There was a problem hiding this comment.
These should be in double quotes. There are a few single quotes throughout.
erykoff
force-pushed
the
tickets/DM-33957
branch
from
6f77ca7
to
9bfc58f
Compare
No description provided.