Conversation
LennartPurucker
left a comment
LennartPurucker
left a comment
There was a problem hiding this comment.
Added multiple comments. Otherwise LGTM!
openml/datasets/data_feature.py
Outdated
| data_type: str, | ||
| nominal_values: list[str], | ||
| number_missing_values: int, | ||
| ontologies: list[str], |
There was a problem hiding this comment.
I think this needs to have a default value as it is otherwise a breaking change?
There was a problem hiding this comment.
while technically it would make break code, I find it unlikely that people use this constructor outside our library. In the library, I think I have updated all constructors
openml/datasets/functions.py
Outdated
|
|
||
| def data_feature_add_ontology(data_id, index, ontology): | ||
| """ | ||
| Adds an ontology (URL) to a given dataset feature (defined by a dataset id |
There was a problem hiding this comment.
Please add a short description of the concept of ontology in the docstring.
openml/datasets/data_feature.py
Outdated
| number_missing_values : int | ||
| Number of rows that have a missing value for this feature. | ||
| ontologies : list(str) | ||
| list of ontologies attached to this feature |
There was a problem hiding this comment.
Maybe add a short description of the concept of ontologies.
openml/datasets/functions.py
Outdated
| return int(data_id) | ||
|
|
||
|
|
||
| def data_feature_add_ontology(data_id, index, ontology): |
There was a problem hiding this comment.
Requires type hints and return type
openml/datasets/functions.py
Outdated
| return True | ||
|
|
||
|
|
||
| def data_feature_remove_ontology(data_id, index, ontology): |
|
Also, try to fill the PR template so we can document the change for later; thanks! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1313 +/- ##
===========================================
+ Coverage 74.01% 75.82% +1.81%
===========================================
Files 38 38
Lines 5237 5246 +9
===========================================
+ Hits 3876 3978 +102
+ Misses 1361 1268 -93 ☔ View full report in Codecov by Sentry. |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
eddiebergman
force-pushed
the
add_feature_description
branch
from
2b914d0 to
af57407
Compare
for more information, see https://pre-commit.ci
|
Closing and we cherry picked these into a new PR for a simpler times, @janvanrijn you were added to the contributors of that PR! |
|
just for my information: was that PR merged already? (and if not, which PR was it?) |
|
#1316 and it was merged :) |
4 participants
Reference Issue
What does this PR implement/fix? Explain your changes.
How should this PR be tested?
Any other comments?