CABLE-LSM / benchcab

Tool for evaluation of CABLE land surface model
https://benchcab.readthedocs.io/en/latest/
Apache License 2.0
2 stars 3 forks source link

Refactor work_dir creation #171

Closed ccarouge closed 11 months ago

ccarouge commented 11 months ago

Fixes #150

Added a mkdir() wrapper to use throughout the code:

Defined FLUXSITE_DIRS: a dictionary in internal.py that contains all the individual directories for the fluxsite runs.

Change the creation and deletion of fluxsite directory tree and source directory to use relative paths instead of absolute paths.

codecov[bot] commented 11 months ago

Codecov Report

Merging #171 (f083d3e) into main (56448d4) will decrease coverage by 0.40%. Report is 1 commits behind head on main. The diff coverage is 95.83%.

@@            Coverage Diff             @@
##             main     #171      +/-   ##
==========================================
- Coverage   88.97%   88.58%   -0.40%     
==========================================
  Files          26       26              
  Lines        1660     1603      -57     
==========================================
- Hits         1477     1420      -57     
  Misses        183      183              
Files Coverage Δ
benchcab/comparison.py 61.36% <ø> (ø)
benchcab/internal.py 90.69% <100.00%> (+0.22%) :arrow_up:
benchcab/utils/fs.py 100.00% <100.00%> (ø)
benchcab/workdir.py 100.00% <100.00%> (ø)
tests/test_benchcab.py 100.00% <ø> (ø)
tests/test_comparison.py 100.00% <100.00%> (ø)
tests/test_fluxsite.py 100.00% <100.00%> (ø)
tests/test_fs.py 100.00% <100.00%> (ø)
tests/test_workdir.py 100.00% <100.00%> (ø)
benchcab/fluxsite.py 83.13% <87.50%> (+0.09%) :arrow_up:
... and 1 more
SeanBryan51 commented 11 months ago

Doing this made me wonder if we shouldn't define a "FLUXSITE_DIR" dictionary in internal.py instead of creating the list when running benchcab... But it might wait for a redesign of internal.py instead.

Yep it is basically a constant so it could go in internal.py. We could define the relative path list in internal.py and iterate over that?

ccarouge commented 11 months ago

@SeanBryan51 I think we need to discuss our strategy to create/remove directories. For the moment, we are going towards using relative paths to create the fluxsite run tree. But the clean function uses absolute paths:

def clean_directory_tree(root_dir=internal.CWD):
    """Remove pre-existing directories in current working directory."""
    src_dir = Path(root_dir, internal.SRC_DIR)
    if src_dir.exists():
        shutil.rmtree(src_dir)

    run_dir = Path(root_dir, internal.RUN_DIR)
    if run_dir.exists():
        shutil.rmtree(run_dir)

This makes it harder to remember what they do, in particular when writing tests as root_dir becomes redundant in some cases but not others.

Since internal.py contains the relative paths SRC_DIR and RUN_DIR, should we instead write clean_directory_tree with relative paths and handle the location we call it from the outside? Or is it safer to have functions work on absolute paths so they work always the same way no matter how they are called?

SeanBryan51 commented 11 months ago

Since internal.py contains the relative paths SRC_DIR and RUN_DIR, should we instead write clean_directory_tree with relative paths and handle the location we call it from the outside?

Yes. I think ideally we should use relative paths whenever paths are prefixed by the CWD. Maybe we can take a look at this when we fix issue #162?