BradGreig / Hybrid21CM

1 stars 3 forks source link

globals #5

Closed steven-murray closed 5 years ago

steven-murray commented 5 years ago

Having had a look through due to the previous issue about z_heat_max and co, I've realised that almost all of the "global_params" actually only affect a single module (sometimes two). It would seem to me to be better then to separate these into structs which affect each module independently. This will also help with keeping track of which parameter affects which Output Struct, so that the caching is slightly easier.

I think I could implement this by myself, if you think it's a reasonable idea.

BradGreig commented 5 years ago

Yeah, give it a go yourself. If you run into any issues let me know.

It is likely not a final list of these parameters. Any more I'll need to add I'll throw in myself given the framework you set up.

steven-murray commented 5 years ago

I think this is a good idea, but I don't want to dive into it too much without some planning. I also think that implementing it properly will fundamentally change the API, so I'm setting the milestone to v2, to give us time to think about it properly.

My idea is something like the following:

  1. The globals struct should only have stuff in it that needs to be set from python, but which should never be changed (eg. the external_table_path).
  2. Every "main" function (InitialConditions, PerturbField, etc.) should receive two structs worth of inputs: one "params" struct, called <>_params and one "options" struct called <>_options. The former is for any parameter which directly affects this particular function, and is a physically meaningful parameter (something that could potentially be changed in MCMC, if you want to think of it that way). The latter is for any other option that affects the computation (resolution parameters, options, min/max parameters etc.).
  3. These structs should be completely distinct from each other for each function, so that the union of all structs represents all parameters of the calculation, and the intersection of any pair of structs is empty.
  4. The pair of structs for a particular function will be saved to its output struct, so that if that particular output struct is required for another calculation (eg. init_box for the perturb_field), the dependent parameters can all be accessed through it (so we don't end up having to pass all 10 structs to the brightness_temperature function).
  5. Secondary supporting functions (eg. those in ps.c and UsefulFunctions.c) which use "global" parameters would have to be modified to accept either the exact parameters they need, or the struct that contains the set of parameters they need.
  6. This will change the Python wrapper as well: instead of being able to pass all different structs to any of the wrapped functions, the functions will instead receive the dependent output structs, which will themselves include the parameter structs required.

Point 5 is the one that I'm most vague on, and will probably need help with. Before really starting though, I'd really like to gauge your thoughts on this, and try to arrive at the best solution.

The positive aspects (in my mind) to this approach are essentially encapsulation -- each function knows its own parameters exactly. This helps with caching, and provides an easy workflow for the python wrapping (as far as I can see).

BradGreig commented 5 years ago

Yep, I am fine to move this to a later version.

steven-murray commented 5 years ago

Moved to above referenced issue.