[ENH] V1 → V2 API Migration - estimation procedures#1604
[ENH] V1 → V2 API Migration - estimation procedures#1604PGijsbers merged 262 commits intoopenml:mainopenml/openml-python:mainfrom EmanAbdelhaleem:estimation-procedures-migEmanAbdelhaleem/openml-python:estimation-procedures-migCopy head branch name to clipboard
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1604 +/- ##
==========================================
+ Coverage 54.71% 55.61% +0.89%
==========================================
Files 61 63 +2
Lines 5086 5108 +22
==========================================
+ Hits 2783 2841 +58
+ Misses 2303 2267 -36 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
geetu040
left a comment
There was a problem hiding this comment.
This looks great. The implementation is clean and nicely decoupled. The next step should be adding tests, but let's wait until we have a clearer roadmap for that.
Also, please update the PR description to follow this format:
Fixes #...
Depends on #1576
Related to #1575
…into issue1564
geetu040
left a comment
There was a problem hiding this comment.
@EmanAbdelhaleem Thanks for the PR. Nicely done!
I have updated the PR to sync with latest changes in base PR.
@PGijsbers please review/merge.
PGijsbers
left a comment
There was a problem hiding this comment.
I had a closer look at the old code. Best I can tell list_estimation_procedures is never called nor is it actually public (or am I wrong?). The PR also keeps some code duplication that existed in the old style. I would refactor it as follows:
- remove the current
listfunction and renamelist_detailedtolist. Going forward, the only way to access the estimation procedures is with all their metadata. It seems that besides the retention of metadata, the only difference is in how to assess that the server response is parseable, and I don't think those differences matter (especially since it looks likelist_estimation_proceduresisn't used). - define an EstimationProcedure named tuple which has fields
id,name,task_type_id, andtype. Make thelistfunction return that instead. - in
_get_estimation_procedure_listyou can convert the named tuples with._asdict() - in
list_estimation_proceduresjust map to the names of the returned values to keep the response identical
This makes sure that at this stage we already reduce duplicate code, and that in the future we easily refactor further to make use of the typed return values instead.
| Fetches estimation procedures from the v1 XML API endpoint. | ||
| """ | ||
|
|
||
| def list(self) -> builtins.list[str]: |
There was a problem hiding this comment.
Can you try again? It really shouldn't. If it does and you don't understand why or don't know how to best resolve it, please let us know.
…reAPI.list everywhere
|
@PGijsbers Thanks for the detailed suggestions. I've applied the changes. This PR is up for review/merge. |
PGijsbers
left a comment
There was a problem hiding this comment.
will merge if CI looks good
Fixes #1622
Depends on #1576
Related to: #1575
Details
This PR implements
EstimationProceduresresource, and refactor its existing functions