Skip to content
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

Rename argument to be more intuitive #796

Merged
merged 3 commits into from
Oct 8, 2019
Merged

Conversation

mfeurer
Copy link
Collaborator

@mfeurer mfeurer commented Oct 2, 2019

What does this PR implement/fix? Explain your changes.

Renames argument id of function openml.evaluations.list_evaluations to run to make it clearer that the IDs are for the run.

How should this PR be tested?

There is an existing unit test for this functionality. I updated at to reflect the changed argument name.

@mfeurer mfeurer requested a review from ArlindKadra October 7, 2019 15:22
@codecov-io
Copy link

Codecov Report

Merging #796 into develop will increase coverage by 0.56%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #796      +/-   ##
===========================================
+ Coverage    87.71%   88.28%   +0.56%     
===========================================
  Files           36       36              
  Lines         4208     4438     +230     
===========================================
+ Hits          3691     3918     +227     
- Misses         517      520       +3
Impacted Files Coverage Δ
openml/evaluations/functions.py 93.91% <100%> (+1.91%) ⬆️
openml/_api_calls.py 83.11% <0%> (-1.3%) ⬇️
openml/datasets/functions.py 95.96% <0%> (+0.48%) ⬆️
openml/runs/functions.py 84.25% <0%> (+2.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f461732...cdd6188. Read the comment docs.

Comment on lines 18 to 23
task: Optional[List] = None,
setup: Optional[List] = None,
flow: Optional[List] = None,
run: Optional[List] = None,
uploader: Optional[List] = None,
tag: Optional[str] = None,
Copy link
Collaborator

@PGijsbers PGijsbers Oct 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going for more intuitive names, should they be changed to be plural, as they expect a list? Or perhaps even better, plural and _id suffix (e.g. run_ids), to indicate we are looking for something that parses to an id and not actual OpenMLX (e.g. OpenMLRun) objects instead?

Do we also want to change the typehint to Optionl[List[Union[str, int]]]? It's pretty verbose, but more complete... Maybe it would be enough to include this in the docstrings.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be happy to change those, but I think this is for a future PR while this is about renaming an argument from id to run. Shall I open an issue so we don't forget about this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just did this so I can merge & close PR. See #804.

@PGijsbers PGijsbers merged commit a32f556 into develop Oct 8, 2019
@PGijsbers PGijsbers deleted the add_run_to_list_evaluation branch October 8, 2019 09:12
@PGijsbers
Copy link
Collaborator

I didn't find a related open issue that should be closed by this PR.

@mfeurer
Copy link
Collaborator Author

mfeurer commented Oct 8, 2019

I didn't find a related open issue that should be closed by this PR.

There is none. I tried to create an example and found the resulting code unintuitive so I just went ahead.

Thanks for merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants