-
Notifications
You must be signed in to change notification settings - Fork 63
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tunable CCL global settings at the Python-level #918
Conversation
Pull Request Test Coverage Report for Build 2781756053
💛 - Coveralls |
…he C-level; makes the entire PR shorter and simpler
…unc without cosmo
@nikfilippas Do you want to rebase/merge master and I then rewrite the warning to match the new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, thanks a lot @nikfilippas . First review. Main thing is you need to explain to me this object
business you're using in parameters.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, a couple of new comments and a couple old ones remaining.
One overall comment: in an ideal world I'd have preferred to start by cleaning up the C layer so actually none of this need to live there. This would simplify a lot of the code you've had to add here, but c'est la vie. I think this is what we'll do in the future.
…nto global_ccl_params
@damonge back to you. I agree with your overall comment. CCL has to be re-written in C++ like @tilmantroester has suggested, but changing the entire internal structure will require a lot of work. And at that point, why not simply write the whole thing in a language like Julia and just use the Python layer as a lightweight wrapper. This PR does the job in what I believe is an elegant way, but at some point how CCL handles the parameters needs to change. |
@tilmantroester @nikfilippas are we happy to leave C-level modifications for a future PR and merge this one? If so, I can approve. |
@damonge I think there is consensus now. |
@nikfilippas can you resolve the conflicts? I'll approve then when the tests pass. |
@damonge Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This PR decouples the (otherwise connected at the C level) CCL accuracy parameters from
Cosmology
objects, at the Python level.In a new module
parameters.py
I have implemented the classCCLParameters
which wraps the SWIG-generated classes for the spline parameters, the GSL parameters, and the physical constants.In the spirit of making
Cosmology
objects truly immutable (which they ought to be) I have deprecated the option to mutate the accuracy parameters of any instantiatedCosmology
: all new instances are populated with the global parameters at the time they are created.Users can access (and set) the global parameters using
ccl.gsl_params
,ccl.spline_params
, andccl.physical_constants
.To do this I removed the
const
type definition of the C-level spline and GSL parameters, so they are added inccllib.cvar
by SWIG. Physical constants were already mutable (becauseT_CMB
is temporarily mutated inCosmology
).This feature comes with the advantage that we can remove all dummy cosmologies from the codebase. Every time we need one just to look up the accuracy parameters, we can just peek at the parameters at the time of instantiation, if a
Cosmology
is not provided.Maybe it's also worth considering having something like
ccl.spline_params.set_accuracy("high" | "medium" | "low")
with pre-made parameter sets. High accuracy would be what is expected from LSST-like analyses with slower computation, while low accuracy will give a considerable increase in speed. This is one of the things mentioned in #588.