-
Notifications
You must be signed in to change notification settings - Fork 12
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
[DOC] Replace use of "estimator" term in base object interfaces with more general references #293
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #293 +/- ##
==========================================
- Coverage 85.07% 84.29% -0.78%
==========================================
Files 45 45
Lines 3015 3184 +169
==========================================
+ Hits 2565 2684 +119
- Misses 450 500 +50 ☔ View full report in Codecov by Sentry. |
@fkiraly I think this is ready to merge/review 👍 |
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.
Mostly agreed, I have one issue that imo we need to resolve.
In some occurrences, I anticipate that using "object" to refer to a class - namely one inheriting from BaseObject
- will cause a lot of confusion.
This is because there are python objects - instances of classes - so we are overloading a term that has a much more general meaning.
The most risky instances of these are the case of "object class" or "base object class", because the thing in question is neither a class, nor a base class..
These occcurences - in set_tag
, get_tag
, get_tags
, set_params
, get_params
I would leave as is, unless we find a better solution. Perhaps we can work with "from self
" etc, but that might confuse users who aren't too familiar wit the self
concept.
Some solution suggestions:
get_tags
,get_tag
- just say "get tags from object and dynamic tag overrides" etc. Or change it completely, to "get tag value, with key-wise inheritance", then explain inheritance in another sentence.get_class_tags
,get_class_tag
- "from class, with key-wise inheritance"?set_params
- I've removed the "base" (commit above), I think that fixes the ambiguity since we're not mentioning "class"
Other comments:
create_test_instance
, does not create an instance ofBaseObject
, but ofcls
- "sub-estimator" sounds strange. Component objects?
@tpvasconcelos, are you still working on this? |
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.
Made changes to clarify docs; reverted changes to _clone
to avoid a divergence from sklearn
.
Reference Issues/PRs
Depends on #281
What does this implement/fix? Explain your changes.
See #281 (comment) and relevant replies.
What should a reviewer concentrate their feedback on?
PR checklist
For all contributions
the PR topic is related to enhancement, CI/CD, maintenance, documentation, or a bug.
For code contributions