Bugfix/wigner6j integer sum #148
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In addition, I added some code to detail which triads were failing the test. The text is added to the `msg` string with a single call to print if any of the tests fail. It would be nice to have some global verbosity flag to allow the user to turn on/off the message. Finally, I offer two error messages (coincidentally) of the same length.
No problems with the logic in the triangularity test. This commit just adds messaging in the same vein as the integer check.
return 0 for a 6j that violates triagularity return nan for a 6j with non-integer traid sum(s)
For consistency with prior code, suppress feedback on 6j triangularity and integer sum rule violations unless explicitly requested.
|
Hi @petermao thank you for your efforts on this update! Is there anything you want to add on this? From my side, we should just remove unreachable code (that raises error now after |
nikolasibalic
approved these changes
Sep 30, 2023
| print(msg) | ||
| return np.nan | ||
| raise ValueError("6j-Symbol is undefined when any triad has a non-integer sum") | ||
| raise ValueError("6j-Symbol is defined only when all triads have integer sums") |
There was a problem hiding this comment.
Let's remove unreachable code here (two raise statements after return)
There was a problem hiding this comment.
I will fix this in main after merging, before releasing new version.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Integer sum check in Wigner-6j code was not catching non-integer sums.
1b9eb26:
5a5b1d8: add similar messaging to triangularity test
b77b250: bypass errors by returning sensible values when tests fail (I acknowledge that the
nancould be controversial)