-
Notifications
You must be signed in to change notification settings - Fork 91
Edit components API to match other modules #747
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
Codecov Report
@@ Coverage Diff @@
## master #747 +/- ##
=======================================
Coverage 99.34% 99.34%
=======================================
Files 148 148
Lines 5175 5175
=======================================
Hits 5141 5141
Misses 34 34
Continue to review full report at Codecov.
|
@@ -124,57 +124,26 @@ Pipeline Plot Utils | |||
Components | |||
========== | |||
|
|||
Transformers |
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 removed all the subheadings because I thought they were unnecessary and cluttered the API reference.
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.
Got it. I know this was added recently; I could see both sides. But I think what you've got here is good. We don't offer any one-sentence summaries elsewhere in the API doc. And right now, we don't have a ton of components, so perhaps the extra level of organization is unnecessary as you said. If we add enough components to warrant further editing here, I'll be pretty excited! 😂
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 personally really liked the added info and structure, but we can add it back later once we have more components since it does feel a little empty with just one component per category.
I like it because it makes the api reference slightly more educational
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.
If we're thinking of adding them back later might be prudent just to keep them there so we don't have to recreate the work! Mainly just thought it looked weird cause there were a lot of text for one class and it wasn't done anywhere else.
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.
Hmm, good point @kmax12 . Perhaps we can find middle ground.
I'd advocate for deleting the categorization of components by type, but still keeping the high-level headings and one-sentence summaries.
So, to be specific:
- "Component Base Classes" section with
ComponentBase
,Transformer
andEstimator
, and with a one-sentence description of what components are. - "Transformers" section with a one-sentence description of what transformers are.
- "Estimators" section with "Classifiers" and "Regressors" sub-sections, and a one-sentence description for each of those.
@jeremyliweishih does that sound good to you too?
|
||
Imputers | ||
-------- | ||
ComponentBase |
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.
Can we list Transformer
and Estimator
here too?
@@ -7,6 +7,8 @@ | |||
|
|||
|
|||
class ComponentBase(ABC): | |||
"Base class for 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.
Nice
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! I do have one request: I think we should list Transformer
and Estimatorunder the
Component Base Classes` heading.
@jeremyliweishih : RTD failed with this:
Oh, and now I see |
@dsherry yeah! Trying to fix it. |
@dsherry I added to the descriptions. Lmk what you think! |
@jeremyliweishih looks great! I left some suggested tweaks. |
Fixes #742.