🧑🏭🔢 Fix numeric triples factory #862
Conversation
trigger ci
trigger ci
trigger ci
trigger ci
| @@ -932,13 +941,15 @@ def from_labeled_triples( | |||
| @classmethod | |||
| def from_path( | |||
| cls, | |||
| *, | |||
There was a problem hiding this comment.
this seems to conflict with the fact that you removed the path from the usages of TriplesFactory.from_path in the examples. Typically, I still leave the first required argument before the * since common usage might not have the keyword
There was a problem hiding this comment.
I added the path keyword parameter
There was a problem hiding this comment.
On second thought, this looks a bit redundant since the function is already called from_path.
| cls, | ||
| *, | ||
| triples: LabeledTriples, | ||
| numeric_triples: LabeledTriples = None, |
There was a problem hiding this comment.
Is there a meaningful case in which numeric_triples could be none? I see it raises an error, but why even allow this? Is this so it can follow the parent's signature?
There was a problem hiding this comment.
Is there a meaningful case in which numeric_triples could be none?
no
Is this so it can follow the parent's signature?
yes
There was a problem hiding this comment.
So if you want to leave it as is, then this is good to merge
trigger ci
This PR is a spin-off of #857 , which extracts the changes to numeric triples factory.