SimonEnsemble / Xtals.jl

working with crystal structures
MIT License
21 stars 5 forks source link

Quieter init and JULIA_DEPOT as the default for `set_paths` #161

Closed ven-k closed 2 years ago

ven-k commented 2 years ago
eahenle commented 2 years ago

Thanks @ven-k for the PR!

Have to think about using depots1() (the .julia directory) as the default... makes sense, but will affect existing workflows. Setting no_warn=true is definitely good for the init case.

Tests fail because Pkg isn't being imported. Also the version bump needs to be to at least 0.4.3 😉, and if we change the actual set_paths behavior I would make it 0.5.0, and the docs will need to be updated as well.

@SimonEnsemble thoughts? I'm leaning towards making the default path live at depots1() but keep the set_path default as pwd, with a separate option depot::Bool to set paths in the depot.

ven-k commented 2 years ago

set_path default as pwd, with a separate option depot::Bool to set paths in the depot

Having both will run into problem when path_to_data=pwd(), depot=true is set.

Have to think about using depots1()

Sure. LMK your final thoughts, I'll make the changes.

Pkg isn't being imported

Oops. Will fix 'with the rest of changes

SimonEnsemble commented 2 years ago

the no_warn is a nice addition, but I honestly like using pwd() as the default file path, and this is how all of our projects are currently set up. in my experience it can be confusing to have all files in one place since different projects use different force fields, crystal structures, etc. especially putting it in a hidden folder maybe not good? we like to work in some directory and pull data from a data folder nested in that directory so everything is there for that project. I think this is much better for reproducibility too b/c then the github repo will contain the data folders as well...

codecov-commenter commented 2 years ago

Codecov Report

Merging #161 (afa6cc7) into master (4e9e6f4) will not change coverage. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #161   +/-   ##
=======================================
  Coverage   93.67%   93.67%           
=======================================
  Files           8        8           
  Lines         980      980           
=======================================
  Hits          918      918           
  Misses         62       62           
Impacted Files Coverage Δ
src/Xtals.jl 100.00% <100.00%> (ø)

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 4e9e6f4...afa6cc7. Read the comment docs.