Dear Watchers – forgive my unceremoniously adding you to this ticket, but I wanted to ask for some design advice and it seemed like posting the following essay to HipChat would be excessive!

HSC has changed the default flux measurement used by PhotoCal from PSF to aperture flux. Per discussion on HipChat, the consensus is that this change should propagate to LSST. It's trivial to do this by simply pointing the (new) CalibFlux slot at a CircularApertureFlux field: base_CircularApertureFlux_4 would be equivalent to the 12 pixel flux.naive used on HSC in the default configuration.

However, that directly ties the CalibFlux to the vagaries of the CircularApertureFlux algorithm configuration. For example, the “4” is a reference to a particular entry in a list of radii; adding another radius to the CircularApertureFlux configuration might unintentionally change the CalibFlux.

An alternative would be to add way to automatically choose the appropriate entry in the list of radii based on a radius specified in the task configuration. But that would tie the CalibFlux into the specifics of CircularApertureFlux implementation: It would be more awkward to simply put the PsfFlux in the slot instead, for example.

Another alternative would be to simply define a NaiveFlux measurement, which would consist of running CircularApertureFlux with a single aperture. Changing the aperture used for NaiveFlux would more directly and obviously couple with the value used for CalibFlux. But it would mean we were effectively running the CircularApertureFlux plugin twice, with different configurations, which seems less than ideal.

Thoughts?

It seems like what we really want is a persistent way to refer to a particular aperture in the list produced by CircularApertureFlux - i.e. something more like dict access instead of list access. That'd make the code a lot easier to read, too.

The problem with that is of course the fact that the radii are floating point numbers, and don't have to be integers at all (though at present they are). I don't have a great solution for that, but I'd be willing to accept a somewhat ugly solution (which I haven't thought about deeply), since it would make so many other things work better.

Yeah, that was my first thought in response to Jim's comments. I'm not sure if that's too ugly for him to even consider, though...!

It also only solves part of the problem: as well as changing the list of radii, the unsuspecting user could also change the photocal by futzing with the maxSincRadius conf parameter to CircularApertureFlux. That may not be as important, though; given that we use the sinc algorithm only when we think it'll be fast enough, I'm guessing its impact on the output is modest. (But I've not checked.)

I'm not too worried about the use of maxSincRadius. I'd change the emphasis of your statement as to when we use it: we don't use it only when we're certain the impact on the output is negligible. But it's the radius that really matters here in terms of remaining persistent, not the algorithm we use. Most importantly, I think we'd never measure the same radius both ways and use them for different purposes.

Rounding off the radius isn't necessarily too ugly for me...it's more a question of which code is responsible for doing the rounding, and how that interacts with the way the fields are named (note that the integer index is currently part of the field name, and dropping a floating point number in that is a bit problematic).

Rounding off the radius isn't necessarily too ugly for me...it's more a question of which code is responsible for doing the rounding, and how that interacts with the way the fields are named (note that the integer index is currently part of the field name, and dropping a floating point number in that is a bit problematic).

Being foolhardy (ambitious? simplistic?), I simply switched the format string to:

std::string prefix = (boost::format("%s_%.1f") % name % ctrl.radii[i]).str();

and the basics seem to be working. What issues do you foresee here? The documentation implies round tripping through FITS will work ("."s should become "_"s). There's extra functionality for [] access based on periods, but I don't think that should actually cause this to break.

Actually, we no longer convert "." to "_" in I/O (if we did, we'd never no what to convert back), and I'd like to avoid having periods in field names, so I think we need to convert the period to an underscore in that format string. That might do everything we need.

Could you point me at the out-of-date documentation that says we're still doing the conversion?

Well, that is sad!

I just spoke to Robert Lupton who is worried about burning time on this when he thinks that ultimately configuration will need to be substantially rethought anyway. As a heuristic, I'll dump in underscores and see if everything Just Works before the weekend. If it spills over, let's go for the simplest solution (using base_CircularApertureFlux_4) and start mulling over what larger-scale fixes we might make to the way we handle configuration.

Could you point me at the out-of-date documentation that says we're still doing the conversion?

I was looking at https://lsst-web.ncsa.illinois.edu/doxygen/x_masterDoxyDoc/afw_table.html:

By convention, field names are all lowercase and have '.'-separated elements. Only letters, numbers and periods should be used. These rules are not enforced, but names that do not meet these requirements may not round-trip correctly in FITS (periods are converted to underscores in the FITS persistence layer, so we cannot distinguish between the two when we read tables from FITS).

(Of course, it should have been obvious that this was out of date, since we're pervasively using _ in field names.)

The code changes to make this work as discussed above are predictably straightforward, although quite a few test cases across the stack need to be adjusted to account for them.

We end up with some constructs that look like this

self.initialMeasurement.algorithms["base_CircularApertureFlux"].radii = [7.0]

[... other code ...]

self.initialMeasurement.slots.apFlux = "base_CircularApertureFlux_7_0"

where previously we could have used base_CircularApertureFlux_0. Any changes will therefore have to be made in two places (which is bad); on the other hand, changes in one place can't inadvertently cause unexpected changes elsewhere (which is good).

I think overall this is a net win, but only just; if anybody doesn't like it, let's not bother. In either case, I agree with Robert Lupton that this is not worth burning more time on.

I agree this is a bit better. I'm a bit surprised it's not more disruptive, but if it isn't, I'm all for it.

Changes on:

• meas_base
• meas_algorithms
• pipe_tasks
• ip_diffim
• obs_sdss
• lsst_dm_stack_demo

Note that the first four of the above are relative to DM-3106, not master. Further, this involves a change to the lsst_dm_stack_demo repository which can't be checked by Buildbot (or Jenkins) until after it has been merged, since they only pull master. It works fine locally, though.

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?

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.