Skip to content

Issue #0: Remove slow hash. #169

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

Closed
Mark2000 wants to merge 1 commit into from
Closed

Issue #0: Remove slow hash. #169

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions src/bsk_rl/scene/targets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,6 @@ def id(self) -> str:
self._id = f"{self.name}_{id(self)}"
return self._id

def __hash__(self) -> int:
Copy link
Copy Markdown
Contributor

@LorenzzoQM LorenzzoQM Aug 9, 2024

Choose a reason for hiding this comment

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

To check if I understood it right, the hash will now be generated based on the entire object, and not only on its id, correct?

Copy link
Copy Markdown
Contributor Author

@Mark2000 Mark2000 Aug 9, 2024

Choose a reason for hiding this comment

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

Yeah, I'm not sure why I put this in in the first place.

Copy link
Copy Markdown
Contributor Author

@Mark2000 Mark2000 Aug 9, 2024

Choose a reason for hiding this comment

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

Actually, it may be worth checking how targets are copied, if ever. I believe deepcopy changes the hash but not the id, so this could potentially lead to issues. I'll take a closer look at some point; not a big enough performance improvement to warrant potentially introducing an error without being sure of this.

Copy link
Copy Markdown
Contributor

@LorenzzoQM LorenzzoQM Aug 9, 2024

Choose a reason for hiding this comment

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

I hadn't considered that before, but I agree. This is a subtle change that can lead to unexpected consequences.

"""Hash target by unique id."""
return hash((self.id))

def __repr__(self) -> str:
"""Get string representation of target.

Expand Down