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.

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

Jump to bottom

🪓🪑 Fix TriplesNumericLiteralFactory split #594

Merged
merged 12 commits into from
Oct 12, 2021
Merged

🪓🪑 Fix TriplesNumericLiteralFactory split #594

merged 12 commits into from
Oct 12, 2021

Conversation

Rodrigo-A-Pereira
Copy link
Contributor

@Rodrigo-A-Pereira Rodrigo-A-Pereira commented Sep 23, 2021 •
edited by cthoyt
Loading

Closes #593

Link to the relevant Bug(s)

This pull request is relative to the TriplesNumericLiteralFactory splitting bug mentioned on issue #593.

Description of the Change

The fix was done with small changes to the TriplesNumericLiteralFactory class.

  • Overwriting the clone_and_exchange_triples that it had from the parent (TriplesFactory). The new clone_and_exchange_triples returns a TriplesNumericLiteralFactory to which it is passed the numeric_triples of the original TriplesNumericLiteralFactory that is being spliced.

  • TriplesNumericLiteralFactory now has a new optional constructor parameter numeric_triples.

  • numeric_triples is now an attribute of TriplesNumericLiteralFactory that can be accessed from outside.

Possible Drawbacks

I don't think there is any drawback deriving from this change.

Verification Process

Running the code example given of issue #593, now correctly returns:

TriplesNumericLiteralsFactory(num_entities=8, num_relations=2, num_triples=7, inverse_triples=Falsenum_literals=3)
TriplesNumericLiteralsFactory(num_entities=8, num_relations=2, num_triples=2, inverse_triples=Falsenum_literals=3)

Release Notes

Make it so that when using split on a TriplesNumericLiteralFactory object the return objects are also TriplesNumericLiteralFactory objects.

@cthoyt
Copy link
Member

cthoyt commented Sep 23, 2021

Would you please add a minimal unit test that shows this works now? Somewhere in https://github.com/pykeen/pykeen/blob/master/tests/test_triples_factory.py is probably fine

@Rodrigo-A-Pereira
Copy link
Contributor Author

Would you please add a minimal unit test that shows this works now? Somewhere in https://github.com/pykeen/pykeen/blob/master/tests/test_triples_factory.py is probably fine

Yes, of course!

@Rodrigo-A-Pereira
Copy link
Contributor Author

Hi @cthoyt,
I added a simple test (test_triples_numeric_literals_factory_split), that verifies if the new objects maintain the correct class and if their numeric_literals attribute is passed correctly to the clones. Not sure if this is enough, I don't have many experience in writing tests.

@cthoyt
Copy link
Member

cthoyt commented Oct 12, 2021

@PyKEEN-bot test please!

@cthoyt
Copy link
Member

cthoyt commented Oct 12, 2021

@PyKEEN-bot test please

@cthoyt cthoyt added the bug Something isn't working label Oct 12, 2021
@cthoyt cthoyt changed the title Fix TriplesNumericLiteralFactory split 🪓🪑 Fix TriplesNumericLiteralFactory split Oct 12, 2021
@cthoyt cthoyt merged commit 23d1d63 into pykeen:master Oct 12, 2021
@cthoyt
Copy link
Member

cthoyt commented Oct 12, 2021

Thank you @Rodrigo-A-Pereira for being patient! It seems everyone on the team was a bit preoccupied lately so this took longer than expected. We really appreciate the bug fix :)

@Rodrigo-A-Pereira
Copy link
Contributor Author

Hi @cthoyt,
I'm glad to be able to contribute to this amazing library, even if it's just in this small way. Keep up the good work! 😄

@cthoyt cthoyt mentioned this pull request Oct 13, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unexpected behaviour when using split() on a TriplesNumericLiteralsFactory object.
3 participants
@Rodrigo-A-Pereira @cthoyt @PyKEEN-bot