-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
burn python2.x bridges more #12948
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
burn python2.x bridges more #12948
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -203,7 +203,7 @@ class _PGNMF(NMF): | |
def __init__(self, n_components=None, solver='pg', init=None, | ||
tol=1e-4, max_iter=200, random_state=None, | ||
alpha=0., l1_ratio=0., nls_max_iter=10): | ||
super(_PGNMF, self).__init__( | ||
super().__init__( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All changes to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An easier solution might also be to wait until #12812 is merged |
||
n_components=n_components, init=init, solver=solver, tol=tol, | ||
max_iter=max_iter, random_state=random_state, alpha=alpha, | ||
l1_ratio=l1_ratio) | ||
|
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.
All changes of the form
don't really bring anything and should be reverted IMO. It's one character less, for the cost of creating merge conflicts with all the open PRs.
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.
they do offer an improvement and were added in python2.7 -- numbering format specifiers is human-error-prone operation
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.
Yes and it's good to use them going forward, but adding merge conflicts for this seems not useful.
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 think you could say the same about using
super()
-- what's the hangup 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.
Fair, that would be an argument to keep the super calls the same. But I do feel the new way makes them more readable. The "hangup" is that every code change brings maintenance costs with it and we're trying to balance the benefit of making the change with that cost.
This is the reason why we discourage changes that are largely cosmetic, like applying pep8.
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 don't really have that strong an opinion, though...