8000 __eq__ method for Corr class by fjosw · Pull Request #206 · fjosw/pyerrors · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

__eq__ method for Corr class #206

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Jul 19, 2023
Merged

__eq__ method for Corr class #206

merged 6 commits into from
Jul 19, 2023

Conversation

fjosw
Copy link
Owner
@fjosw fjosw commented Jul 18, 2023

I added a __eq__ method for the Corr class for simpler checks like corr == 0.

@fjosw fjosw requested a review from s-kuberski July 18, 2023 12:39
@s-kuberski
Copy link
Collaborator

Hi, thanks for adding this. I think it will be useful but I have to understand in which cases we expect and equality to be evaluated as True. In your test, you check
0 * mcorr + scorr[2] == scorr[2]
which compares, timeslice by timeslice, an array with a scalar. I guess there are only very rare cases where this would evaluate to True anyways, but I just wondered if it is a well-posed question to check equality here.

@fjosw
Copy link
Owner Author
fjosw commented Jul 19, 2023 •
edited
Loading

My initial idea for implementing __eq__ was to check whether the difference is zero on every timeslice leaning on the comparison functionality of the Obs class. Based on this comparisons are possible for every object that can be subtracted from a correlator.

I then noticed a few subtleties with None entries as these are ignored by the mathematical operations. For this reason, I wrote a special case for the comparison of two correlators where the None entries are explicitly checked. All other operations still fall back to the difference so the correlator can be compared with int, float, Obs, an array of Obs etc. although you are right, that the last two cases rarely evaluate to true.

@s-kuberski
Copy link
Collaborator

That makes sense. I just wondered about the "formal definition" of two objects being equal and if someone could be confused because two objects that are not equal (because they are different objects) would be determined to be equal based on this routine, which checks the numerical difference, timeslice by timeslice. I think both variants are fine.

@fjosw
Copy link
Owner Author
fjosw commented Jul 19, 2023

I agree with you that this behavior can be confusing. I now rewrote the comparison to return a boolean array that has the same shape as the correlator. To get a single bool one could now use

(corr1 == 0).all()
(corr1 == corr2).all()

The new behavior is closer to np.ndarray which I prefer as we set up the Corr class to be an array container with a dedicated 0th dimension. What do you think?

@s-kuberski
Copy link
Collaborator

Thanks. I think it is good that the new behavior is more transparent to the user at the cost that the direct comparison of two correlators needs the extra call to np.all(). The similar behavior to arrays is certainly good.

@fjosw fjosw merged commit af28f77 into develop Jul 19, 2023
@fjosw fjosw deleted the feat/corr_eq branch July 19, 2023 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@s-kuberski s-kuberski Awaiting requested review from s-kuberski

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fjosw @s-kuberski
0