Repository navigation
Confusing run_flow_on_task #451
Description
I've seen several people make the same error when using OpenML. They do
run_flow_on_task(flow, task)
instead of
run_flow_on_task(task, flow)
I can see how this is confusing to beginners. Would it make sense to rename the method? Or at least add more info to the error message? Right now it returns with
AttributeError: 'OpenMLTask' object has no attribute 'model'
This makes sense, but could we also add something like 'Please check whether you entered the task and flow objects in the right order'?
Metadata
Metadata
Assignees
Labels
No labels
Activity
PGijsbers commentedon Apr 23, 2018
This is something I find myself doing wrong still, every now and then. What is the reasoning behind this order anyway?
I would suggest making sure the order in the method name is the same as the order of the parameters (no preference as to doing it by renaming the method or re-ordering the parameters). Alternatively you could also just allow both permutations of arguments (but this requires additional input validation and might not be as neat).
mfeurer commentedon Apr 23, 2018
There is no reasoning behind this order, it's sloppiness. I asked @ArlindKadra to do this once dataset uploading works. I'd actually prefer to allow both permutations. If @ArlindKadra didn't yet start on this, do you want to take this over @PGijsbers ?
PGijsbers commentedon Apr 23, 2018
In principle yes, but I'm also about to go on holiday. Considering it shouldn't be much work, I'll see if I can squeeze it in.
mfeurer commentedon Apr 23, 2018
Cool!
PGijsbers commentedon Apr 23, 2018
Do we want to phase out usage of the old ordering (e.g. give a deprecation warning for now)? If we want to keep supporting both orders, I'm a little torn. On the one hand, I think we should notify the user when we interpret the arguments as swapped, because otherwise errors might become increasingly confusing. On the other hand, in a normal workflow it would be nicer to just swap silently.
edit: And do we which ordering do we want in the documentation? I think preferably we should advocate one way (even if both are possible (for now)).
edit2: And I assume we will also add similar behavior for
run_model_on_task?joaquinvanschoren commentedon Apr 24, 2018
Allowing both permutations and outputting a warning for the 'wrong' order seems a good way to go. Documentation should only document the more logical/consistent option IMHO.
Indeed, the same goes for
run_model_on_task.ArlindKadra commentedon Apr 24, 2018
@mfeurer I have not started on this, so there is no duplicate work happening :)
janvanrijn commentedon Apr 26, 2018
My personal preferences:
My proposal would be to simply change the order of arguments (you seem to agree that it makes sense) and throw an error if someone uses the old order of arguments.
mfeurer commentedon Jun 8, 2018
Fixed in #452.
PGijsbers commentedon Feb 25, 2019
I am working on #498 and while it's not necessary to remove it, should we do it anyway? It's been 8 months now, so it feels safe to me. Throwing a clear
TypeErrorin the case that the old order is used, of course.mfeurer commentedon Feb 25, 2019
I recently found that this our old OpenML100 paper has an example using the wrong argument order. Therefore, I would like to keep it as long as the OpenML100 paper is updated.
PGijsbers commentedon Feb 25, 2019
Sounds reasonable.