-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
DOC Fix documentation of the base module #17548
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
Conversation
I am wondering whether:
|
if there are constraints in the code, then yes.
changing the interface of existing public classes is tricky and we tend to avoid.
if the input is converted to the desired input type regardless of the input type then the input's type doesn't matter.
it does help in some cases, but please be mindful of the number of hyperlinks when you do it since since it can make the doc look less readable. |
Thanks for the review @adrinjalali! I have applied the suggested changes and a few that I think are helpful. |
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.
LGTM otherwise.
n_rows : int | ||
Number of rows in the bicluster. | ||
|
||
n_cols : int | ||
Number of columns in the bicluster. |
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'm not sure if this is less confusing than the shape: tuple
for the users. WDYT @NicolasHug
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 in general we separate the entries like that (numpy does it too)
For 2-uples it's OK to merge them too IMHO
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 decided to separate in two entries because is the approach used in the get_indices
method. Nevertheless, LGTM any of these approaches.
Which approach do you prefer?
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 just have a doubt regarding the BaseEstimator
. We started to use estimator instance
in other places. I would really think that we should add the rule in the contributing guideline.
@adrinjalali do you recall anything about this case |
No I don't, but I know I myself would write "estimator instance" instead of "BaseEstimator" just because I find it more intuitive for users, but I really don't mind either of them. |
+1 for "estimator instance" as well. The existence of BaseEstimator is an implementation detail for most users. |
+1 of using I will apply these changes! |
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Reference Issues/PRs
Related with #3791.
What does this implement/fix? Explain your changes.
This PR fixes the documentation of the
sklearn.base
module for consistency.