Skip to content

Fix 1124: provide clear naming for cache directories #1254

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.

Sign up for GitHub

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

Jump to bottom
Merged
PGijsbers merged 4 commits into from
Jun 13, 2023
Merged

Fix 1124: provide clear naming for cache directories #1254

PGijsbers merged 4 commits into from
Jun 13, 2023

Conversation

@mfeurer
Copy link
Collaborator

@mfeurer mfeurer commented Jun 12, 2023

Reference Issue

Fixes #1124

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

  • Introduce the concept of a root_cache_directory, i.e. the cache directory in which the concrete cache directories for the different servers reside
  • Rename the function set_root_directory to set_root_cache_directory
  • Improve the docstring of both functions

How should this PR be tested?

Regular unit tests.

Any other comments?

No.

mfeurer added 3 commits June 12, 2023 17:37
@mfeurer
Fix #1124
8742e19 
Make variable `openml.config.cache_directory` private so that
there is no confusion on how to retrieve the cache directory
(since this should be done via `openml.config.get_cache_directory`)
@mfeurer
Improve docstrings and method names
cd00d43
@mfeurer
Rename base_ to root_
f296309
PGijsbers
PGijsbers requested changes Jun 12, 2023
View reviewed changes
Copy link
Collaborator

@PGijsbers PGijsbers left a comment

Choose a reason for hiding this comment

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

See comments.

@codecov-commenter
Copy link

codecov-commenter commented Jun 12, 2023 •
edited
Loading

Codecov Report

Patch coverage: 100.00% and project coverage change: +2.91 🎉

Comparison is base (a4ec4bc) 81.27% compared to head (8e2de47) 84.19%.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop    #1254      +/-   ##
===========================================
+ Coverage    81.27%   84.19%   +2.91%     
===========================================
  Files           38       38              
  Lines         5010     5010              
===========================================
+ Hits          4072     4218     +146     
+ Misses         938      792     -146
Impacted Files Coverage Δ
openml/config.py 82.18% <100.00%> (+0.57%) ⬆️
openml/testing.py 84.43% <100.00%> (ø)

... and 12 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mfeurer mfeurer requested a review from PGijsbers June 13, 2023 07:50
PGijsbers
PGijsbers approved these changes Jun 13, 2023
View reviewed changes
Copy link
Collaborator

@PGijsbers PGijsbers left a comment

Choose a reason for hiding this comment

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

LGTM

@PGijsbers PGijsbers merged commit a7f2639 into develop Jun 13, 2023
@PGijsbers PGijsbers deleted the fix_1124 branch June 13, 2023 13:19
github-actions bot pushed a commit that referenced this pull request Jun 14, 2023
Matthias Feurer: Fix 1124: provide clear naming for cache directories (…
fc0b575 
…#1254)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@PGijsbers PGijsbers PGijsbers approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Provide clear naming for the inconsistent ways to get the cache directory

4 participants

@mfeurer @codecov-commenter @PGijsbers