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

Updates the definition of graph isomorphism #140

Merged
merged 2 commits into from
Jan 19, 2025
Merged

Updates the definition of graph isomorphism #140

merged 2 commits into from
Jan 19, 2025

Conversation

hartig
Copy link
Contributor

@hartig hartig commented Jan 17, 2025

.. as proposed in #128, including the additional change proposed in #128 (comment)

Notice that I also removed the "that is a node of G" part of bullet point 2 and 3. This change is needed for the new recursive part of the definition to be correct (i.e., when applying M to IRIs or literals inside triple terms).

Additionally, I also changed the last (now fifth) bullet point by putting M(p) in the second triple mentioned in this bullet point. While this change is not strictly necessary (because p is an IRI and M maps each IRI to itself), using M here looks a bit more consistent.


Preview | Diff

@hartig hartig requested review from gkellogg, afs, pchampin and pfps January 17, 2025 16:28
Copy link
Contributor

@pchampin pchampin left a comment

Choose a reason for hiding this comment

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

LGTM.

I notice that M was also applied to the predicate of (asserted) triples and triple terms, whereas the original definition kept p as is. Since p is always an IRI, this is absolutely equivalent, but more verbose...

I have a slight preference for the terse version (p instead of M(p)), but I can live with both.

@hartig
Copy link
Contributor Author

hartig commented Jan 17, 2025

I have a slight preference for the terse version (p instead of M(p)), but I can live with both.

I'm fine with changing it back to p if others prefer that as well. However, in this case, I would also change the M(p) in the new fourth bullet point to p. Just to be consistent.

@afs
Copy link
Contributor

afs commented Jan 17, 2025

I prefer M(p) because it extends to generalized RDF (and symmetric RDF if we have to add blank node predicates for sementics).

Also, it looks clearer to me because it is mapping p even if it only ever results in p.

Copy link
Contributor

@pfps pfps 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

@hartig
Copy link
Contributor Author

hartig commented Jan 19, 2025

Given the approvals of all other editors, and also of @pfps who created the issue that this PR addresses, I will merge the PR now.

@hartig hartig merged commit 7293161 into main Jan 19, 2025
3 checks passed
@hartig hartig deleted the Issue128 branch January 19, 2025 21:42
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.

5 participants