Skip to content

Conversation

jbrea
Copy link
Collaborator

@jbrea jbrea commented Dec 6, 2022

Addresses #20.
Thanks @maleadt. I didn't know about Scratch.jl. I think this is much cleaner now.

@codecov-commenter
Copy link

codecov-commenter commented Dec 6, 2022

Codecov Report

Merging #21 (5bae5d7) into dev (ec9cd80) will decrease coverage by 6.76%.
The diff coverage is 100.00%.

❗ Current head 5bae5d7 differs from pull request most recent head fd0a277. Consider uploading reports for the commit fd0a277 to get more accurate results

@@            Coverage Diff             @@
##              dev      #21      +/-   ##
==========================================
- Coverage   30.37%   23.61%   -6.77%     
==========================================
  Files           1        2       +1     
  Lines          79       72       -7     
==========================================
- Hits           24       17       -7     
  Misses         55       55              
Impacted Files Coverage Δ
src/OpenML.jl 100.00% <100.00%> (ø)
src/data.jl 21.42% <100.00%> (-8.96%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@maleadt
Copy link

maleadt commented Dec 6, 2022

Yeah, that's better. Is there a reason you don't just encode the datasets as a static Artifacts.toml (you can set them to lazy to avoid downloading them all at package installation time)?

@jbrea
Copy link
Collaborator Author

jbrea commented Dec 6, 2022

OpenML has nearly 5k datasets right now and new datasets are constantly added. I prefer the Scratch.jl approach over having to update an Artifact.toml every time when new datasets are added.

@maleadt
Copy link

maleadt commented Dec 6, 2022

Makes sense! FWIW, the advantage of artifacts is that they are easily cached, while scratchspaces are more ephemeral.

@jbrea
Copy link
Collaborator Author

jbrea commented Dec 21, 2022

@ablaom I think this is ready to be merged.

@ablaom ablaom merged commit 9d5abc3 into JuliaAI:dev Dec 23, 2022
@ablaom ablaom mentioned this pull request Dec 23, 2022
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.

4 participants