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

Feature #753 #932

Merged
merged 37 commits into from
Sep 2, 2020
Merged

Feature #753 #932

merged 37 commits into from
Sep 2, 2020

Conversation

PGijsbers
Copy link
Collaborator

Looking for feedback. For the example I still need to finalize how the predictions are generated/formatted to make it clear and not distract from the overall example.

It is incomplete as while trying to explain how to format the
predictions, I realized a utility function is required.
Previously the description text that accompanies the prediction file was
auto-generated with the assumption that the corresponding flow had an
extension. To support custom flows (with no extension), this behavior
had to be changed. The description can now be passed on initialization.
The description describing it was auto generated from run_task is now
correctly only added if the run was generated through run_flow_on_task.
@PGijsbers PGijsbers requested a review from mfeurer July 10, 2020 08:58
openml/runs/run.py Show resolved Hide resolved
openml/runs/functions.py Show resolved Hide resolved
examples/30_extended/custom_flow_tutorial.py Outdated Show resolved Hide resolved
examples/30_extended/custom_flow_tutorial.py Outdated Show resolved Hide resolved
examples/30_extended/custom_flow_tutorial.py Outdated Show resolved Hide resolved
examples/30_extended/custom_flow_tutorial.py Outdated Show resolved Hide resolved
examples/30_extended/custom_flow_tutorial.py Outdated Show resolved Hide resolved
examples/30_extended/custom_flow_tutorial.py Outdated Show resolved Hide resolved
examples/30_extended/custom_flow_tutorial.py Outdated Show resolved Hide resolved
examples/30_extended/custom_flow_tutorial.py Outdated Show resolved Hide resolved
@PGijsbers
Copy link
Collaborator Author

Thanks for the feedback :) will process after the meeting.

I am not for each field what the specifications are.
In particular:
 - text changes
 - fetch true labels from the dataset instead
PGijsbers and others added 9 commits July 13, 2020 13:35
* list evals name change

* list evals - update
* adding config file to user guide

* finished requested changes
* version1

* minor fixes

* tests

* reformat code

* check new version

* remove get data

* code format

* review comments

* fix duplicate

* type annotate

* example

* tests for exceptions

* fix pep8

* black format
* Preliminary changes

* Updating unit tests for sklearn 0.22 and above

* Triggering sklearn tests + fixes

* Refactoring to inspect.signature in extensions
* Add flake8-print in pre-commit config

* Replace print statements with logging
* fix edit api
@PGijsbers PGijsbers marked this pull request as ready for review August 14, 2020 12:39
@codecov-commenter
Copy link

codecov-commenter commented Aug 14, 2020

Codecov Report

Merging #932 into develop will increase coverage by 0.98%.
The diff coverage is 96.29%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #932      +/-   ##
===========================================
+ Coverage    87.65%   88.64%   +0.98%     
===========================================
  Files           37       37              
  Lines         4383     4941     +558     
===========================================
+ Hits          3842     4380     +538     
- Misses         541      561      +20     
Impacted Files Coverage Δ
openml/runs/functions.py 83.28% <95.45%> (+0.45%) ⬆️
openml/runs/run.py 87.98% <100.00%> (+0.09%) ⬆️
openml/datasets/functions.py 94.34% <0.00%> (+0.50%) ⬆️
openml/evaluations/functions.py 92.95% <0.00%> (+0.72%) ⬆️
openml/extensions/sklearn/extension.py 93.05% <0.00%> (+1.87%) ⬆️
openml/datasets/dataset.py 85.66% <0.00%> (+4.08%) ⬆️

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 6b245bd...5ef24ab. Read the comment docs.

@PGijsbers
Copy link
Collaborator Author

@mfeurer I don't see any reduced coverage, (where) am I reading the codecov report wrong?

@mfeurer
Copy link
Collaborator

mfeurer commented Aug 14, 2020

I think it complains that not enough of the diff is tested. 76% of your diff is tested, while codecov expects 87.65% of the diff being tested. I guess the 87.65% is our current test coverage.

@PGijsbers
Copy link
Collaborator Author

Ah, I was looking at the wrong tab 😓 looks like I'll have to add some tests that check the error cases / learning curve tasks. Thanks!

Copy link
Collaborator

@mfeurer mfeurer left a comment

Choose a reason for hiding this comment

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

Hey Pieter, I'm afraid I have some more questions on this example (and a few minor change requests)

openml/runs/functions.py Outdated Show resolved Hide resolved
tests/test_runs/test_run_functions.py Outdated Show resolved Hide resolved
tests/test_runs/test_run_functions.py Outdated Show resolved Hide resolved
tests/test_runs/test_run_functions.py Outdated Show resolved Hide resolved
tests/test_runs/test_run_functions.py Outdated Show resolved Hide resolved
examples/30_extended/custom_flow_tutorial.py Outdated Show resolved Hide resolved
examples/30_extended/custom_flow_tutorial.py Outdated Show resolved Hide resolved
examples/30_extended/custom_flow_tutorial.py Show resolved Hide resolved
examples/30_extended/custom_flow_tutorial.py Show resolved Hide resolved
PGijsbers and others added 2 commits August 28, 2020 17:05
Also throw NotImplementedError instead of TypeError for unsupported task
types. Added links in the example.
* change edit_api to reflect server

* change test and example to reflect rest API changes

* tutorial comments

* Update datasets_tutorial.py
It is incomplete as while trying to explain how to format the
predictions, I realized a utility function is required.
Previously the description text that accompanies the prediction file was
auto-generated with the assumption that the corresponding flow had an
extension. To support custom flows (with no extension), this behavior
had to be changed. The description can now be passed on initialization.
The description describing it was auto generated from run_task is now
correctly only added if the run was generated through run_flow_on_task.
I am not for each field what the specifications are.
In particular:
 - text changes
 - fetch true labels from the dataset instead
Also throw NotImplementedError instead of TypeError for unsupported task
types. Added links in the example.
@PGijsbers
Copy link
Collaborator Author

PGijsbers commented Sep 1, 2020

I did a rebase on develop (because edit api tests failed), locally it looked like it worked fine. I thought the old commits should've simply been replaced by the ones patched onto the develop head? (i.e. same code diff, different commit id)
edit: I can try to fix this, but I guess we are squashing anyway so it doesn't matter much?

@mfeurer
Copy link
Collaborator

mfeurer commented Sep 1, 2020

I thought the old commits should've simply been replaced by the ones patched onto the develop head?

Yeah, I don't know how to do this probably either, I'm usually rebasing to avoid such hassle.

I can try to fix this, but I guess we are squashing anyway so it doesn't matter much?

No, shouldn't matter.

Shall I do a final review and then merge?

@PGijsbers
Copy link
Collaborator Author

PGijsbers commented Sep 1, 2020

Yeah, I don't know how to do this probably either, I'm usually rebasing to avoid such hassle.

I normally merge because it's less of a hassle. After the failed rebase I actually found the general advise not to rebase if the work already lives on remote.

Shall I do a final review and then merge?

That would be greatly appreciated! The Travis fails seem to be on the openml server side.

@mfeurer mfeurer merged commit a442688 into develop Sep 2, 2020
@mfeurer mfeurer deleted the feature_#753 branch September 2, 2020 06:38
@mfeurer mfeurer mentioned this pull request Sep 2, 2020
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.

8 participants