-
-
Notifications
You must be signed in to change notification settings - Fork 213
[DOC] Enhance Docstrings of Flows Core Public Functions #1569
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1569 +/- ##
==========================================
- Coverage 53.02% 52.72% -0.31%
==========================================
Files 36 36
Lines 4326 4326
==========================================
- Hits 2294 2281 -13
- Misses 2032 2045 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1a9b652 to
de22ee7
Compare
SimonBlanke
left a comment
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.
Good job. Just a few questions in the code.
| Side Effects | ||
| ------------ | ||
| - None: results are fetched and returned; no local state is modified. |
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.
Is this true? Are there local connection states? And what about caching? 'no local state is modified' is a strong claim.
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 meant that it's a read-only operation that doesn't change variables, database or files. I am not sure about flows cuz I haven't gone very deep with its implementation, However, in evaluations, the list function has no caching as far as I know, and I think mostly this is the case for flows too.
Would you suggest modifying this to
None: read-only 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.
'Read only' would be clear. It is easier to understand
| ``strict_version`` is True, an exception will be raised. | ||
| strict_version : bool, optional (default=True) | ||
| When ``reinstantiate`` is True, whether to enforce exact version | ||
| requirements for the extension/model. If False, a fallback flow may |
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.
It is not a fallback flow though, is it? It is a new flow:
openml-python/openml/flows/functions.py
Line 137 in de22ee7
| return new_flow |
Fallback would suggest some sort of 'backup' flow. Is this the case here (it might be)?
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.
nope it's not the case, I meant by fallback that it's an alternative flow, but I think fallback might be misleading, I think I will just make it new flow
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.
Okay new flow then. It is better to be more precise.
Metadata
Details
enhance the docstrings of flows core public functions, add examples, parameter default, parameter type..etc