remove USE_TS_FLUCT

[steven-murray]

The USE_TS_FLUCT option is redundant and confusing. Clearly it is not necessary in ComputeTsBox, and in ComputeIonizationBox we also pass do_spin_temp. This mean we have to do extra checks and its not simple for the user.

I recommend removing USE_TS_FLUCT entirely. This should come down to replacing all occurences of it with do_spin_temp. I can do this, as long as you have no objections, @BradGreig .

[BradGreig]

This depends on how you want to structure the whole code. USE_TS_FLUCT was to be held within the flag_options struct, containing information regarding the chosen options by the user. If you want to separate out the spin temperature part, then technically you should be separating out all user defined options (recombinations, mass-dependent ionising efficiency, RSDs etc.). It doesn't make sense to me that spin temperature should be separated from this. As such, my preference would be for it to remain in the flag_options struct.

[steven-murray]

That's true. I understand the desire to leave all "options" in some common dictionary/struct that can be easily viewed and passed around. I like this idea overall, but I find it clunky to have only one dictionary of "options" for the entire workflow, rather than one per "stage". I think conceptually, the best approach is to have two dictionaries per stage (i.e. ICs, perturb field, ionize, spin temp, brightness temp). One should contain parameters (conceptually things that could potentially be constrained, practically, anything that is a float), and the other should contain options (either ints or bools, or maybe strings if necessary). The latter controls how the model runs, while the former controls the precise values.

This gives granularity, so that we know that changing an option in the spin temp stage doesn't affect the ICs or perturb field, and changing an option in the ionize stage doesn't affect the spin temp etc. It also makes it obvious which things can be constrained (in principle) and which things are just model choices.

In this case, I think I'd be happy to let something like "USE_TS_FLUCT" be a part of the ionize options, and remove the do_spin_temp keyword altogether. But this will have to be checked in the Python routine -- the other way to use spin temperature is literally just to pass a TsBox to the ionize() function. This is in some way the most natural way to ask for spin temperature, but we need to also allow for a dynamic calculation within the function.

It feels wrong to me to pass FlagOptions containing USE_TS_FLUCT to the spin temperature function itself. It is redundant and it would confuse me if I was a user. On the other hand, the choice of "please do spin temperature" is an attribute of the ionization box -- the box would be different if the choice is different. So I think it should contain that information.

[BradGreig]

I think this greatly depends on how you conceptualise it. In my mind, the vast majority of users should only really be running run_lightcone or run_coeval, in which case these specific details are lost to the user. Any of these redundancies etc. that you refer to are only down on the absolute lowest level individual function calls.

Through the same line of argument, then inhomogeneous recombinations should only be passed to the ionize() function, whereas subcell RSDs should only be passed to the brightness_temperature() function. In doing so, it then requires additional keyword arguments to all those functions that the user must define.

Although it might be redundant, I think centralising it makes the user experience easier as they only have to centrally define it, and then the code automatically takes care of the rest.

[catherinewatkinson]

I strongly agree with Brad on this one. The simpler it can be from a user point of view the more people will make use of your code. I really think make it as easy as humanly possible for someone to download it and just start running it out of the box. It also simplifies the job of maintaining the user instructions massively.

My preference as a user is that everything user changeable is dealt with by setting and passing everything dictionaries (or all as classes, I don't care as long as it is the same and it's quick to work out how to do it). Would also be cool to make it a little clearer how to run it in the various levels of parametrisation. i.e. How a user runs it under the old parametrisation no Ts - new parametrisation no Ts - old parametrisation with Ts - new parametrisation with Ts

[steven-murray]

OK so maybe my pedantism is not so useful in this case :-) Brad is right, that although the lower-level functions would have a more obvious and clean interface if the options were separated, the user almost always only deals with a wrapper of the wrapper, in which it's probably just easier to have one "options" dictionary. I am usually dealing with the lower-level functions so my idea of how things get used is a bit biased :-).

For now, based on these things, I'm going to start removing all dangling parameters in functions, and instead using conglomerated dict/structs exclusively.

However, for future reference, it is possible to have the best of both worlds. That is, we could ask the user who interacts only with the top-level function to only pass a single options dict that contains everything. BUt then that wrapper of wrappers will pass out the individual bits to more granular structs that get passed to the lower-level functions. Whether this extra work is worth it I don't know!

[BradGreig]

Yeah, this is what I was going to suggest. Internally to the outer wrapper layer, you can deconstruct the dict/struct to enable the lower level functions to have the transparency you desire.

Provided on the user side it is straightforward (i.e. just a bunch of options), I don't care so much how it looks internally.

[steven-murray]

Cool. I'll go ahead and ensure the outermost later is as simple as possible, and then later we can dissect the internals a bit more!

On Thu, Feb 7, 2019, 4:34 PM BradGreig <notifications@github.com wrote:

Yeah, this is what I was going to suggest. Internally to the outer wrapper layer, you can deconstruct the dict/struct to enable the lower level functions to have the transparency you desire.

Provided on the user side it is straightforward (i.e. just a bunch of options), I don't care so much how it looks internally.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/BradGreig/Hybrid21CM/issues/25#issuecomment-461635835, or mute the thread https://github.com/notifications/unsubscribe-auth/ABNo3tOxPfKcKPac04WJvye_S0BeLZ1Wks5vLLf4gaJpZM4aiMjN .

[steven-murray]

Done in 82ecc0fe3a3eb6295b57bf39c39a2cfd894d3778.