-
Notifications
You must be signed in to change notification settings - Fork 69
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
Refactor pyccl under an abstract CCLObject. #934
Conversation
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 few more comments and I'll start with the individual classes and then tests.
Also, have you checked what's happening to the tests? |
@damonge tests should be fine now. |
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.
Last few comments before I move on to the tests.
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.
Quick comments on tests
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.
Alright, few comments on fancy_repr. Thanks for implementing it.
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.
Alright, final few comments before merging!
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, can you merge master after the latest GHA branch merge? Then, when all tests pass, I'll approve.
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 is part 2 of the refactoring proposal. It implements a
CCLObject
which all other base classes inherit from. The aim of this is to be able to centrally control the behavior of every type defined in pyccl, as well as to be able to adopt a uniform framework for object comparison.The main idea is that the developer only needs to define a
__repr__
for the class, and__hash__
and__eq__
will be automatically added. This way, we can check equivalence of any object to any other object, and it is up to the developer to decide the equivalence criteria when writing up the__repr__
method.Because some objects contain a large amount of data (such as
Pk2D
- contains huge arrays), hashing is a relatively expensive operation. Therefore,hash
andrepr
for the object are cached the first time they are requested.But, what if we want to update some parameters? Won't the cached
repr
be wrong then?All
CCLObject
s are immutable by default, unless anupdate_parameters
method is defined. Only through there may the instance mutate, and when it does, the caches are deleted and recomputed on request. Alternatively, there are two ways to mutate an object: either via theUnlockInstance
context manager, or via the@unlock_instance
decorator.Docstrings in
base.py
explain more thoroughly the details of this implementation.Secondary changes
In an attempt to improve the OOP code structure, I made two more major changes:
HaloProfile
,Concentration
,HaloBias
, andMassFunc
(andEmulator
in PR906) cannot be instantiated because they contain abstract methods.__init__
method of the parent class, as there is no point that these live as different types. The subclasses converted to functions are (i)CosmologyVanillaLCDM
, (ii) all subclasses of theMassDef
superclass, (iii) all subclasses of theTracer
superclass. These changes don't affect how users interact with CCL; this is all internal restructure of the codebase.How to review this
I suggest starting from
CCLObject
inbase.py
, then having a look at_repr.py
. Everything else relies on these two things.There are more than 100 changed files, but most of them are in
tests/
andbenchmarks/
, where I've re-written the imports to take advantage of caching and run them faster. This version passes all the tests on my PC and I have been using it for a while, so if you are a developer feel free to install it and play around.Note: This PR relies on 918, 923, 925, and 933. Hopefully once these are through the review will be much more straightforward as
git diff
from the default branch will only contain the relevant changes.Branch Dependency Visualization