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

Clarify definition of unique parent condition #39057

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

user202729
Copy link
Contributor

@user202729 user202729 commented Nov 29, 2024

As in the title.

I don't think "unique parent condition" or "unique parent behavior" was defined anywhere previously, and this definition is to the best of my understanding (just come across it recently).

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

Copy link

github-actions bot commented Nov 29, 2024

Documentation preview for this PR (built with commit 5a339b9; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

Copy link
Collaborator

@mantepse mantepse 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, although I'd love to have a brief look by @nbruin or @tscrim.

@@ -517,6 +517,9 @@ class that inherits from :class:`UniqueRepresentation`: By adding
simultaneously inherit from :class:`CachedRepresentation` and from
:class:`~sage.misc.fast_methods.WithEqualityById`.

If the class is also a :class:`~sage.structure.parent.Parent`,
then we says it satisfies the *unique parent* condition.
Copy link
Contributor

Choose a reason for hiding this comment

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

"says": typo

@nbruin
Copy link
Contributor

nbruin commented Dec 27, 2024

What I see presently on this PR are some minor corrections and modifications. They seem fine, but I don't see how the task set in the title is completed. There is still no definition of what the "unique parent condition" is. Is this ticket supposed to be ready for review?

I don't think it's the case that "Unique Parent Condition" means "Parent"+"UniqueRepresentation").
I think "Unique Parent Condition" really just means "equal means identical" (for a parent), in the way that WithEqualityByID establishes. It arises from the need in the coercion framework to determine equality of parents really quickly, so it just relies on identity in those cases. The coercion framework would be completely happy with parents that do not have any caching but where non-identical instances are not equal. It's really only the "independently reconstructible" (that from the same parameters an equal parent can be constructed) requirement that causes the caching for UniqueFactory and UniqueRepresentation.

@user202729 user202729 marked this pull request as draft January 5, 2025 07:07
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.

3 participants