DM-37193: Reorganize vectorActions #108
Conversation
taranu
force-pushed
the
tickets/DM-37193
branch
2 times, most recently
from
c598c2e
to
2cc6728
Compare
| @@ -21,17 +21,12 @@ | |||
| from __future__ import annotations | |||
|
|
|||
| __all__ = ( | |||
There was a problem hiding this comment.
If you're trying to match the order they appear in the file, this __all__ looks like it should be:
__all__ = (
"LoadVector",
"DownselectVector",
"MultiCriteriaDownselectVector",
"ConvertUnits",
"CalcSn",
"ConvertFluxToMags",
"MagDiff",
"ExtinctionCorrectedMagDiff",
"RAcosDec",
"PerGroupStatistic",
"ResidualWithPerGroupStatistic",
)
|
|
||
|
|
||
| class LoadVector(VectorAction): | ||
| """Load and return a Vector from KeyedData.""" | ||
| class ConvertFluxToMags(VectorAction): |
There was a problem hiding this comment.
Either this should be named ConvertFluxToMag or ConvertFluxesToMags. I prefer the singular myself.
| class LoadVector(VectorAction): | ||
| """Load and return a Vector from KeyedData.""" | ||
| class ConvertFluxToMags(VectorAction): | ||
| """Turn nano janskies into magnitudes.""" |
There was a problem hiding this comment.
"nanojanskies", one word.
| return vecA / vecB | ||
|
|
||
|
|
||
| class FractionalDifference(VectorAction): |
There was a problem hiding this comment.
Most vector actions in the mathActions file end with the suffix Vector (all except here and ConstantValue). Is it worth considering making that consistent here too?
| _LOG = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class ConstantValue(VectorAction): |
There was a problem hiding this comment.
Most vector actions in the mathActions file end with the suffix Vector (all except here and FractionalDifference). Is it worth considering making that consistent here too? E.g., ConstantVector? ConstantValueVector?
There was a problem hiding this comment.
I was actually considering renaming it to Scalarsomething rather than Constantsomething, but decided against it.
But to the extent that there is a naming scheme, the Vector-suffixed actions all begin with verbs (see also LoadVector and DownselectVector in vectorActions.py), whereas the unsuffixed actions are nouns (ConstantValue, MagDiff as short for MagnitudeDifference, etc). In principle you could verbify the various Diff(erence) actions since difference is a verb too, but I don't think the names would be any clearer. For ConstantValue, I can't think of a good way to verbify it at all. ReturnScalar? Actually, LoadScalar would be consistent with LoadVector, even though it's not really loading anything.
There was a problem hiding this comment.
Fair enough. In which case, I suggest leaving this (and the other non-conformant action here) as-is for the time being. Further changes can be punted to a future ticket, if desired.
No description provided.