Update ValenceDict #1185
Update ValenceDict #1185
Conversation
lilyminium
force-pushed
the
refactor-valence-dict
branch
from
e50d2b1 to
d6a8322
Compare
There was a problem hiding this comment.
Cool - Thanks @lilyminium! I left some comments, but none are blocking, so this is good to merge as-is. Though if you have time it'd be great to also make similar classmethod/staticmethod changes in ImproperDict.index_of (no need for me to re-review after that, you can merge those changes directly). But since static and class methods are so similar, that's not blocking to merging this PR.
The releasenotes look good. I kinda waffled on whether these are API breaking changes, but I don't think static <--> classmethod changes need to be reported, so they should be in good shape.
Also, super nice tests - I'm still confused how the valencedict ordering problem never got caught, but I'm glad that this PR puts in tests to ensure they remain well behaved!
| permutations = OrderedDict( | ||
| { | ||
| (refkey[0], refkey[1], refkey[2], refkey[3]): 0, | ||
| (refkey[3], refkey[1], refkey[2], refkey[0]): 1, |
There was a problem hiding this comment.
Thanks for catching the issue with this line in #1183. I've spent a while thinking of how this could possibly be correct and am pulling a blank. So I agree with you and @trevorgokey that it's a bug. I'm wondering how this has never come up, or if it was making trouble in production and never got caught.
There was a problem hiding this comment.
This method may not be used anywhere, to be honest. If you grep index_of the only instances seem to be associated with VirtualSites, which either uses and ImproperDict or the virtual site one.
| # Ensure key is a tuple. | ||
| key = tuple(key) | ||
| assert len(key) > 0 and len(key) < 5, "Valence keys must be at most 4 atoms" | ||
| # Reverse the key if the first element is bigger than the last. |
There was a problem hiding this comment.
(not blocking) This PR opts to delete a lot of comments, a few of which are indeed incorrect, but many of which are correct-but-only-marginally-useful. In the future I'd kinda appreciate having fewer marginally-useful comments removed.
There was a problem hiding this comment.
I was always taught not to have low-level comments that are immediately obvious from the nearby code, like Ensure key is a tuple for tuple(key)-- both because they introduce unneeded clutter and because they easily go out of date when the code is changed. I thought about leaving some of the comments in the "im/possible" section but then it seemed that most of them were covered in the docstring or associated error messages.
|
Is there anything blocking this from being merged? |
* update ValenceDict * blacken * update changelog * reformat improperdict too Co-authored-by: Jeff Wagner <jwagnerjpl@gmail.com>
Fixes #1183