Apidocs #692
Apidocs #692
Conversation
|
Hi Neeratyoy, Matthias, seems like the unit tests broke because of the release of scipy 1.3.0: which adds this: and breaks this: Is this easy to fix, or shall we update the dependencies to: scipy>0.13.3,<1.3.0? Line 45 in 4152f91 |
|
@joaquinvanschoren thanks for having a look at this. I'd actually prefer to use liac-arff for this instead. First, it's much faster now since Joel Nothman from the sklearn team improved it, and second, we'd use the same arff reader in all places. However, I suggest that we use a different PR for that. @Neeratyoy could you please put that on your todo list? |
|
@mfeurer alright I shall look into this first. |
openml/study/__init__.py
Outdated
| @@ -1,4 +1,4 @@ | |||
| from .study import OpenMLStudy, OpenMLBenchmarkSuite | |||
| from .study import OpenMLStudy, OpenMLBenchmarkSuite, BaseStudy | |||
There was a problem hiding this comment.
Please do not make BaseStudy available to the user here.
There was a problem hiding this comment.
Removed.
Pushed the changes.
There was a problem hiding this comment.
Sorry, but this one is still there.
There was a problem hiding this comment.
Yes, sorry my mistake. Just noticed. Pushed the changes.
openml/tasks/task.py
Outdated
| Parameters | ||
| ---------- | ||
| task_type_id : int or str |
There was a problem hiding this comment.
Seing this I'm surprised that we allow strings here and cast them to int inside the constructor. Could you please update the code so that:
- the casting happens outside
- the docstring reflects this
- the type annotation reflects this, too
There was a problem hiding this comment.
By casting outside, are you referring to an explicit cast made by the user or in any function invoking the constructor? Just checked some other constructors, and they accept ids only as int.
Shall I make the id parameters take only int here too?
There was a problem hiding this comment.
I mean 'any function invoking the constructor'. Yes, it would be good to only accepts integers here, too.
What does this PR implement/fix? Explain your changes.
How should this PR be tested?
Any other comments?