Fix Version/s: None
Sprint:Science Pipelines DM-S15-5
Team:Data Release Production
- is blocked by
DM-3106 Add slot for calibration flux
This looks good, with one exception. In the python and c++ code you use .1f as a string repeatedly within lines of code. It would be better to avoid these magic numbers and define it in a header file or config file somewhere. This will allow us to change the value in only one location (possibly two if you need to change in python and c++ separately) if we start caring about higher levels of precision (i.e. both 3.2 and 3.28).
Thanks Nate Lust. I see your point. I've attempted to address it by adding a static member function to ApertureFluxAlgorithm which generates field names and is exposed to Python. This means that formatting is defined in exactly one place. I'm not sure that I'd want to go further in terms of exposing this to configuration etc, as it seems like that would be adding a lot of complexity to what should be a simple operation – I'd rather cross that bridge when we come to it.
I'm only semi-happy with the approach adopted, as I don't really like burdening the ApertureFluxAlgorithm interface in this way. Would you mind taking a look to see what you think and if you have any better ideas? Commit is here; I'll squash it into the rest of the PR if & when we decide if this is the approach we actually want to take.
I can't really think of a better place for that function. It seems like the right idea to have that functionality defined in on place. Leaving it where it is seems good, as it can always be moved later if we create a better place for it to live. This seems good to merge provided it passes tests after rebase (sorry the review took so long)
Thanks Nate Lust. Merged to master.
Nate, this is a minor extension to
DM-3106, which you're already looking at, and is based on a design which you help specify. Do you mind taking a quick look?