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

Implement separate __eq__ methods #1033

Merged
merged 23 commits into from
Apr 17, 2023
Merged

Implement separate __eq__ methods #1033

merged 23 commits into from
Apr 17, 2023

Conversation

nikfilippas
Copy link
Contributor

@nikfilippas nikfilippas commented Mar 16, 2023

As @rmjarvis and @marcpaterno correctly pointed out, we can save loads of time comparing CCLObjects if we compare parameters instead of strings. To that end, I extended some functionality that was already there to do parameter comparisons for most objects.

Just like the __repr_attrs__ class variable, which automatically builds a repr using a list of specified arguments, __eq_attrs__ now makes sure that the default __eq__ is overridden and that the individual attributes are compared one-by-one.
For example, in the Cosmology object, we only need to add this line in the beginning:

    __eq_attrs__ = ("_params_init_kwargs", "_config_init_kwargs", "_accuracy_params",)  # check these

    def __init__(...):
        ...

There are only 3 classes where this is not possible (because we have to check the C-level objects): Pk2D, Tk3D, Tracer. For these, I implemented full __eq__ methods as per the usual way.

Timings show that this way is indeed much faster (bld is the repr):
Screenshot from 2023-03-16 04-37-34
Screenshot from 2023-03-16 05-02-53

Note, however, that with the current implementation in master, the first computation of _repr might be slow, but every new call to __eq__ is of the order of nanoseconds (compare to microseconds with this PR proposal). It is a trade-off. So, if we compare objects sequentially (and discard the old one if they are not equal, like in an MCMC) this PR wins. But if we compare one object with several others, the currently implemented way wins.

pyccl/core.py Show resolved Hide resolved
pyccl/base.py Outdated Show resolved Hide resolved
pyccl/pk2d.py Outdated Show resolved Hide resolved
pyccl/tk3d.py Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Mar 20, 2023

Pull Request Test Coverage Report for Build 4719168322

  • 148 of 149 (99.33%) changed or added relevant lines in 13 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.09%) to 97.949%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pyccl/tracers.py 57 58 98.28%
Totals Coverage Status
Change from base Build 4577061095: 0.09%
Covered Lines: 5587
Relevant Lines: 5704

💛 - Coveralls

@nikfilippas nikfilippas mentioned this pull request Mar 23, 2023
27 tasks
@nikfilippas nikfilippas mentioned this pull request Apr 7, 2023
@nikfilippas
Copy link
Contributor Author

nikfilippas commented Apr 11, 2023

Just like with FancyRepr.disable() we can implement central control of __eq__ as well, which can disable __eq__ and use Python's default id-checking. This could potentially prove useful for cases where a developer makes a change but forgets to include a check in __eq__. Instead of overriding it, they could simply do ccl.EqControl.disable() or something like that.

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.

Some comments. Will look at core.py next, then tests

README.md Show resolved Hide resolved
pyccl/base.py Outdated Show resolved Hide resolved
pyccl/base.py Outdated Show resolved Hide resolved
pyccl/base.py Outdated Show resolved Hide resolved
pyccl/halos/massdef.py Show resolved Hide resolved
pyccl/tk3d.py Show resolved Hide resolved
pyccl/tk3d.py Outdated Show resolved Hide resolved
pyccl/tracers.py Show resolved Hide resolved
pyccl/tracers.py Outdated Show resolved Hide resolved
pyccl/tracers.py Outdated Show resolved Hide resolved
@nikfilippas
Copy link
Contributor Author

@damonge all done, back to you now and we can move on with the rest if you are happy with the changes.

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.

Just one (maybe major) comment for core.py

pyccl/core.py Show resolved Hide resolved
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.

Final comments on tests

pyccl/base.py Show resolved Hide resolved
pyccl/tests/test_cosmology.py Outdated Show resolved Hide resolved
@nikfilippas
Copy link
Contributor Author

@damonge back to you

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, we're done. Merge when the tests pass.

@nikfilippas nikfilippas merged commit 1a351df into master Apr 17, 2023
@nikfilippas nikfilippas deleted the CCLObject_eq branch April 17, 2023 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants