AtChem / AtChem2

Atmospheric chemistry box-model for the MCM
MIT License
59 stars 23 forks source link

Use argument parser for smoother input argument handling #378

Closed spco closed 5 years ago

spco commented 5 years ago

Needs some more work to iron out the integration - currently running here to find issues.

codecov[bot] commented 5 years ago

Codecov Report

Merging #378 into master will decrease coverage by 1.35%. The diff coverage is 76.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #378      +/-   ##
==========================================
- Coverage   90.26%   88.91%   -1.36%     
==========================================
  Files          15       17       +2     
  Lines        2117     2256     +139     
==========================================
+ Hits         1911     2006      +95     
- Misses        206      250      +44
Flag Coverage Δ
#build 62.6% <39.87%> (-2.65%) :arrow_down:
#tests 88.07% <70.79%> (-1.54%) :arrow_down:
#unittests 35.85% <35.64%> (+0.85%) :arrow_up:
Impacted Files Coverage Δ
src/dataStructures.f90 95.03% <ø> (ø) :arrow_up:
travis/unit_tests/argparse_test.f90 100% <100%> (ø)
src/atchem2.f90 90.23% <100%> (ø) :arrow_up:
src/argparse.f90 66.66% <66.66%> (ø)
src/inputFunctions.f90 86.31% <88.88%> (-0.61%) :arrow_down:

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 5b274aa...892cf81. Read the comment docs.

spco commented 5 years ago

@rs028 Your thoughts on this would be valued. A rough guide - rather than positional arguments, the atchem executable now takes --model, --output, --reactionRates, --configuration, --constraints, --env_constraints, --photo_constraints, --spec_constraints, --mcm, and --shared-lib.

The 'default' directory structure looks like

                                 model_dir                                          mcm_dir        shared_lib_dir
                                      | 
     +--------------------------------+--------------------------+
     |                                |                          |
 output_dir                     constraints_dir           configuration_dir
     |                                                           |
reactionRates_dir                       +------------------------+----------------------+
                                        |                        |                      |
                              env_constraints_dir   photo_constraints_dir  species_constraints_dir

and the inputs 'cascade down', e.g. setting --constraints=path/to/cons will make atchem look under that for the 3 constraints subdirectories unless their own flag is also supplied.

So now instead of running something like

./atchem2 model/output model/output/reactionRates model/configuration mcm model2/constraints/species model2/constraints/environment model2/constraints/photolysis

we can run

./atchem2 --constraints=model2/constraints

as most are the default locations, apart from the constraints all being held under model2/constraints.

Finally, using the --help flag will display a help message showing the usage, and then exit.

I hope that makes sense! Any feedback very welcome.

spco commented 5 years ago

Fixes #121

rs028 commented 5 years ago

Looks good to me. A few points:

rs028 commented 5 years ago

Related to this: in #121 we also mentioned tidying up the tools directory.

spco commented 5 years ago

1) It's there for completeness, as it is in some ways an input. Users never need to specify it unless theywant to use it from elsewhere (perhaps studying the effects of modifications to the MCM?). Just a thought - the only resl downside is it's an extta flag to read about in the help text. 2) If you're happy that that is always going to be the case then we coluld remove --reactionRates, but again it's there for completeness. 3) Yes, it's for the location of mechanism.so. I'm tempted to change this so that rather than specifying the directory, the user specifies the full path including the .so - that way, users can have a single directory of .so files if they wish, and just link to the appropriate one. The directory itself is of little relevance. 4) Nope.

Let me know what youd like to see on 1)-3).

rs028 commented 5 years ago

1) ok let's keep it 2) i think we can remove it 3) is the fac file also going in there?

Is the same system will be used for the build script?

spco commented 5 years ago

Ok, (1) and (2) are completed. On (3), there's no real answer - the .fac file can be wherever you like, and we use ./tools/build.sh to convert that into a mechanism.so. The input and output directories of that script are different arguments, so the mechanism.so can go anywhere relative to the .fac.

I guess the point I'm making is that currently it's entirely up to the user to find a way to keep track of which .fac relates to which mechanism.so (and mechanism.reac, mechanism.prod etc, whose location is controlled by yet another argument to ./tools/build.sh).

rs028 commented 5 years ago

Okay, good. I don’t understand if reactionRates has been removed or not in the end? Re commit f04477b

spco commented 5 years ago

Yes,--reactionRates is now removed as anargument - I inadvertantly removed too much, as we still want to know where reactionRates_dir is. This should all now be good to go. We can always return to it if we think of a better way to handle the .fac->(mechanism.so,mechanism.prod,mechanism.reac,mechanism.ro2,mechanism.species) transformation.