DM-33993: Correct stellar locus plots for extinction #25
Conversation
yalsayyad
changed the title
yalsayyad
force-pushed
the
tickets/DM-33993
branch
from
60ba8a7
to
a41271b
Compare
|
Have same comments as Eli posted on Slack but otherwise all looks good to me. |
| ebv = df[self.ebv].values | ||
| col1Band = self.magDiff.col1.split('_')[0] | ||
| col2Band = self.magDiff.col2.split('_')[0] | ||
| av1 = self.extinctionCoeffs[col1Band] |
There was a problem hiding this comment.
It might be better with a useful error message here in case someone tries to run it with a filter name not in the dictionary.
There was a problem hiding this comment.
Yes, either a more helpful error or (alternatively) log a warning as above and don't apply extinction corrections. (And below for col2Band).
yalsayyad
force-pushed
the
tickets/DM-33993
branch
5 times, most recently
from
3ae4e58
to
2ac1196
Compare
| default=MagDiff, dtype=DataFrameAction) | ||
| ebv = Field(doc="E(B-V) Column", dtype=str, default="ebv") | ||
| extinctionCoeffs = DictField( | ||
| doc="Dictionary of extinction coefficients for conversion from E(B-V) to extinction, A_filter." |
There was a problem hiding this comment.
This should be A_band. not A_filter.
| def __call__(self, df): | ||
| diff = self.magDiff(df) | ||
| if not self.extinctionCoeffs: | ||
| _LOG.warn("No extinction Coefficients. Not applying extinction correction") |
There was a problem hiding this comment.
This should be _LOG.warning, as warn is deprecated.
| ebv = df[self.ebv].values | ||
| col1Band = self.magDiff.col1.split('_')[0] | ||
| col2Band = self.magDiff.col2.split('_')[0] | ||
| av1 = self.extinctionCoeffs[col1Band] |
There was a problem hiding this comment.
Yes, either a more helpful error or (alternatively) log a warning as above and don't apply extinction corrections. (And below for col2Band).
| return diff | ||
|
|
||
| ebv = df[self.ebv].values | ||
| col1Band = self.magDiff.col1.split('_')[0] |
There was a problem hiding this comment.
Can we always guarantee that the band will be the first part of the split on '_' or does this need to be another configuration parameter?
(Also, I don't have an easy answer to this, but I was getting confused because I was reading col as color but here it means column and has been commented elsewhere we have some naming inconsistencies and technical debt, and is not going to be fixed on this PR.)
There was a problem hiding this comment.
Too late to change the naming convention of the Object Table now!
But if you want the band to be optionally supplied per config parameters you got it.
| class ExtinctionCorrectedMagDiff(DataFrameAction): | ||
| magDiff = ConfigurableActionField(doc="Action that returns a difference in magnitudes", | ||
| default=MagDiff, dtype=DataFrameAction) | ||
| ebv = Field(doc="E(B-V) Column", dtype=str, default="ebv") |
There was a problem hiding this comment.
I think this should be colEbv or something like that, because it's a column not a value.
| if self.magDiff.returnMillimags: | ||
| correction = correction*1000.0 | ||
|
|
||
| return diff - correction*u.mag |
There was a problem hiding this comment.
Upon further review, this is incorrect here and elsewhere (everywhere?) in analysis_drp, I fear. The units should not be u.mag but u.mmag if we're using millimags. This should be a separate ticket (@sr525 ).
There was a problem hiding this comment.
No prob. I changed MagDiff and this Action. I went to give your ColorDiff the same treatment but found that it was just a numpy array, so I'll be leaving the 1000* there.
color_diff = color_diff.to(u.mmag)
AttributeError: 'numpy.ndarray' object has no attribute 'to'
0c01f31
to
fb3ae4a
Compare
per Task naming conventions. Important to do on this ticket which requires config overrides from the obs packages. Config overrides are looked up in <_DefaultName>.py
yalsayyad
force-pushed
the
tickets/DM-33993
branch
from
fb3ae4a
to
da27edd
Compare
No description provided.