Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add
__str__
and__repr__
for components and pipelines #1218Add
__str__
and__repr__
for components and pipelines #1218Changes from all commits
c70a174
1494acd
c272e98
a8fca8a
d74cb20
8fc0c4e
17675ef
a8a6641
b537d0a
418c549
dac5ad8
f0c4013
a27f5fc
5a576d3
14eecc3
57040d2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you want double curly braces instead of triple here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the parameters is a dictionary and contains an expression, I do need all three! The first two are for literal curly braces and the third is for the formatting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm interesting, but then the output (taken from the unit test) ends up as
But ideally I think it should be
Do you agree? The second output could be evaluated in the python repl and turned into an object. I think the first would fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eccabay but, I think its fine to merge this and we can circle back. It adds value even if there's a couple details we may wanna discuss further :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The double curly braces in the unit tests are once again for f-string formatting! If you actually print the repr, you get the second code block you pasted there, and calling eval on the expected_repr produces the correct object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eccabay hmm could you paste an example? I don't follow yet. I thought that
__repr__
returns a string, which requires no further formatting, and that's the end of it 😂 In other words, I thought we should output the second snippet, because that can be pasted into the python REPL and evaluated, but we're currently outputting the first.Again, not blocking merge, I'm just trying to make sure we all agree on desired behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eccabay ok, I think I understand now. So the string I copied was a format string, which is why you need the double curlys there.
I just did a test locally and everything looks great
Realizing the unit tests were using a format string was the key point I was missing, lol. Thanks! 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol thanks for this! I wonder if there's a way to automate this since it's clear that we missed quite a few of our new components (maybe using
all_components()
?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eccabay I just noticed: can we replace
'{final_estimator}':
with'final_estimator':
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't block merge though IMO! Just a detail we should probably chase down
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately the way these tests are written, we can't. The string
__repr__
prints out uses the name of the estimator, so checking for string equality will fail. If I replace{final_estimator}
withfinal_estimator
and tryassert eval(repr(pipeline)) == eval(expected_repr)
, that will pass, but that will reintroduceeval
to the code.The alternative of having
__repr__
outputfinal_estimator
instead of the name of said estimator feels unnecessarily clunky.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! My bad, I didn't notice this was a format string until just now. Got it, makes sense. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonderful, glad we're on the same page 😅. Does this also clear up our other discussion?