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

Fix checking for molecule identity using "is" #167

Merged
merged 1 commit into from
Nov 6, 2024

Conversation

tootea
Copy link
Contributor

@tootea tootea commented Nov 3, 2024

This is necessary to make PLAMS tests compatible with UCS's new content-comparing equality operator.

RFC: Shouldn't we also similarly adjust the mol.atoms comparisons on the immediately following lines? The unit tests work OK even in the current state, but it feels like is not would express the spirit of the assertion better (checking if things are two separate copies or just two references to the same object). Done.

@tootea tootea requested a review from dormrod November 3, 2024 16:14
@tootea tootea self-assigned this Nov 3, 2024
@dormrod dormrod marked this pull request as ready for review November 4, 2024 08:44
Copy link
Contributor

@dormrod dormrod left a comment

Choose a reason for hiding this comment

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

Looks good to me. And yes, I think if you could also just update the atom assertion to be is not that would be better too

This is necessary to make PLAMS compatible with UCS's new
content-comparing equality operator.
@tootea tootea force-pushed the TomasTrnka/ucs_equality branch from 2b0b48c to 48ce118 Compare November 6, 2024 22:08
@tootea tootea changed the title RFC: Fix checking for molecule identity using "is" Fix checking for molecule identity using "is" Nov 6, 2024
@tootea tootea merged commit 649bd73 into trunk Nov 6, 2024
14 checks passed
@tootea tootea deleted the TomasTrnka/ucs_equality branch November 6, 2024 22:12
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.

2 participants