Skip to content
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

Merged
merged 262 commits into from
Mar 15, 2023
Merged

Refactor pyccl under an abstract CCLObject. #934

merged 262 commits into from
Mar 15, 2023

Conversation

nikfilippas
Copy link
Contributor

@nikfilippas nikfilippas commented Apr 28, 2022

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 and repr 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 CCLObjects are immutable by default, unless an update_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 the UnlockInstance 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:

  1. All template classes are now abstract. So, HaloProfile, Concentration, HaloBias, and MassFunc (and Emulator in PR906) cannot be instantiated because they contain abstract methods.
  2. Converted all "special implementation" subclasses to simple functions. These are subclasses that only overload the __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 the MassDef superclass, (iii) all subclasses of the Tracer 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 in base.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/ and benchmarks/, 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

Screenshot from 2022-05-10 18-30-12

Copy link
Collaborator

@damonge damonge left a 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.

@damonge
Copy link
Collaborator

damonge commented Mar 11, 2023

Also, have you checked what's happening to the tests?

@nikfilippas
Copy link
Contributor Author

@damonge tests should be fine now.

Copy link
Collaborator

@damonge damonge left a 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.

Copy link
Collaborator

@damonge damonge left a 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

Copy link
Collaborator

@damonge damonge left a 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.

Copy link
Collaborator

@damonge damonge left a 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!

Copy link
Collaborator

@damonge damonge left a 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.

Copy link
Collaborator

@damonge damonge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

implement the __repr__ method for tracers in python
3 participants