Skip to content

Conversation

@MatthisCl
Copy link
Contributor

@MatthisCl MatthisCl commented Oct 11, 2024

Description:

  • When trying to deepcopy a group, hashfunction is needed before initialization. This leads to an error as self._id is not set before initialization. One could change the hashfunction to not be dependent on the id or change the hashfunction

Issues:

Todos:

Make sure to delete unnecessary points or to check all before merging:

  • Updated Changelog
  • Added / Updated Tests

@markbader markbader self-assigned this Oct 29, 2024
@markbader markbader changed the title add test and change hash function Fix add existing tree to annotation Oct 29, 2024
@markbader markbader marked this pull request as ready for review October 29, 2024 15:23
@markbader markbader requested a review from philippotto December 5, 2024 13:00
Comment on lines 285 to 286
def __eq__(self, other: Any) -> bool:
return type(other) is type(self) and self._id == other._id
Copy link
Member

Choose a reason for hiding this comment

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

this class inherits from group which also defines __eq__, right? couldn't the method here be omitted?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think I tested it before, but it didn't work. I just figured that @attr.define adds a standard __eq__ implementation. This can be avoided when adding @attr.define(eq=False). Thanks for mentioning it, I removed the __eq__ and the __hash__ method from the Skeleton class.

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

great, thank you!

@markbader markbader merged commit 8498db0 into master Dec 9, 2024
19 checks passed
@markbader markbader deleted the fix_add_tree branch December 9, 2024 13:35
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.

Cannot add existing tree to new annotation

4 participants