Remove Recursion #415
Remove Recursion #415
Conversation
| brightness_temp, | ||
| ts_box, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Should these be saved to the struct itself instead of taking the brightness_temp struct's parameters?
There was a problem hiding this comment.
We certainly could do that -- at the moment we always require having a brightness_temp struct in this instance, and the various input structs must be the same for all boxes, so merely referencing them from the brightness_temp struct makes sense to me
| and self.flag_options == other.flag_options | ||
| and self.astro_params == other.astro_params | ||
| ) | ||
|
|
There was a problem hiding this comment.
This is a much older issue but we don't have random seed in __eq__?
There was a problem hiding this comment.
That's true. We should probably ensure that as well -- problematically sometimes you also want to compare without the random seed. But I think you're right that the default equality method should include the random seed.
| "flag_options": inputs.flag_options, | ||
| "init_boxes": initial_conditions, | ||
| }, | ||
| **iokw, |
There was a problem hiding this comment.
So we put the init boxes instead of user/cosmo params in the kwargs?
There was a problem hiding this comment.
Yes -- because every other low-level function other than initial_conditions requires passing initial_conditions instead of the user and cosmo params (because they now require the initial conditions to be passed, which already has those params defined in it, so it's easier to take them from there, rather than having two ways to get the params and having to make sure they match.
src/py21cmfast/drivers/coeval.py
Outdated
| spin_temp=st2 if inputs.flag_options.USE_TS_FLUCT else None, | ||
| z_heat_max=global_params.Z_HEAT_MAX, | ||
| # cleanup if its the last time through | ||
| cleanup=cleanup and z == redshifts[-1], |
There was a problem hiding this comment.
This function doesn't have a cleanup kwarg
There was a problem hiding this comment.
the kwarg has been removed
There was a problem hiding this comment.
ah yeah, because I don't think you need it once recursion is removed
| redshift = redshift.tolist() | ||
|
|
||
| # Get the list of redshift we need to scroll through. | ||
| redshifts = _get_required_redshifts_coeval(flag_options, redshift) |
There was a problem hiding this comment.
consider renaming redshifts and redshift to node_redshifts and out_redshifts
There was a problem hiding this comment.
Great idea -- that would be a lot more clear.
| return redshifts.tolist() | ||
|
|
||
|
|
||
| def _get_coeval_callbacks( |
There was a problem hiding this comment.
I can't find where these are used
There was a problem hiding this comment.
Yeah, I think we've moved away from using these callbacks -- instead we just do a yield from the main wrapper so the user can do anything they like with the result. Perhaps we'd still want something like this to exist for a CLI interface or something where the user has less control over what happens at each step.
|
|
||
| return kwargs | ||
|
|
||
| def __eq__(self, other): |
There was a problem hiding this comment.
Perhaps I'm missing something but should the random seed be included in __eq__?
There was a problem hiding this comment.
Yeah, same reply as above.
| If you know what you are doing, please modify the global parameter: | ||
| NU_X_MAX | ||
| """ | ||
| ) |
There was a problem hiding this comment.
One thing I think would be nice here is to have different running "modes" which are sets of default parameters we can switch between e.g a default parameter set for the halo model, grid model, and constant zeta model
There was a problem hiding this comment.
Absolutely, see #10. But I think not in this PR
testplots/tests.test_integration_features.py--test_power_spectra_coeval[minimize_mem].pdf
Outdated
Show resolved
Hide resolved
Conflicts: src/py21cmfast/__init__.py src/py21cmfast/inputs.py src/py21cmfast/src/21cmFAST.h src/py21cmfast/src/Stochasticity.c src/py21cmfast/wrapper.py src/py21cmfast/wrapper/outputs.py src/py21cmfast/wrapper/structs.py
This PR removes recursion from all the component calculations. It changes a whole bunch of stuff to achieve this well, including:
InputStructobjects that get subclassed toUserParamsetc. These are now attrs-based classes, which makes them a little more transparent.InputParameterclass that handles collections of the InputStruct objects and their validation.