Build script flags and set MCM version

[rs028]

With this PR the build script (build/build_atchem2.sh) is modified to have 4 flags instead of 3 positional arguments:

1. --chemfile= is the fac file (mandatory argument)
2. --configuration= is the configuration directory
3. --mechanism= is the mechanism directory
4. --mcm= is the version of the MCM

This does three things:

1. the location of the configuration (where .config files are) and of the mechanism (where the autogenerated fortran files and the shared library are) can be specified independently
2. the mcm argument used in the previous version of the build script is redefined to indicate the version of the MCM and it is removed from the list of possible executable flags
3. a new file called photolysis-rates is created during the build process: it is a copy of one of the mcm/photolysis-rates_* files, depending on the version of the MCM specificed by the flag argument

This PR resolves issues #297 and #416, making it easier to use older versions of the MCM if needed. It is assumed that:

1. the user has no reason to move the MCM data files from the mcm/ directory
2. the shared library and all the auto-generated files are kept together in the mechanism/ directory, which the user is not supposed to touch (they are rewritten anyways everytime the build script is executed)
3. the user only needs to modify the configuration and parameter files (including customRateFuncs) in configuration/
4. the chemical mechanism file (.fac) can be specified independently with whatever name and in whatever location the user wants.

[rs028]

The new build command is (using the default settings):

.build/build_atchem2.sh --chemfile=model/mechanism.fac --configuration=model/configuration --mechanism=model/configuration/include --mcm=v3.3.1

Provisionally, the model structure implemented in PR #535 is still in place, pending resolution of issue #539.

Also note that the atchem2 executable does not have the --mcm= flag anymore: the executable will simply read the mechanism/photolysis-rates file which has been filled with the data for the correct version of the MCM during the build process.

[rs028]

@AlfredMayhew @bsn502 @willdrysdale @spco : any thoughts on this, before I go ahead and change all the tests?

It would be useful to have a user's prospective, as I don't want to change things just for the sake of it. Is this going to make the model more usable/effective/reliable? Any other suggestion? The names of the flags and default directories can ofc be changed if confusing.

[AlfredMayhew]

Looks good to me @rs028.
Just to check, the default behaviour of the --mechanism flag to the build script is to point to an include subdirectory inside whatever configuration directory has been specified, right? So, if I use .build/build_atchem2.sh --chemfile=mechanism.fac --configuration=new_model/configuration, then this would create the mechanism files in new_model/configuration/include (as opposed to model/configuration/include)?

I don't have any strong feelings about changing the mcm flag from a directory to a version number. I have had to adjust the photolysis files before to add new J values, but I've never felt the need to have multiple mcm directories or anything like that. Presumably anybody who wanted that kind of behaviour could just create new 'MCM versions' for each of their different models, so it could actually be easier than dealing with multiple mcm directories. I think this would be a very niche requirement either way!

[rs028]

No, or rather not yet. at the moment if a flag is not specificed it falls to the default value. So in the case of:
.build/build_atchem2.sh --chemfile=mechanism.fac --configuration=new_model/configuration
the mechanism directory defaults to model/configuration/include. This is provisional however, because the final default settings are not decided (see #539). If we decide that include should not be inside model/configuration, then there is no need to implement the behaviour you describe.

[AlfredMayhew]

Oh, OK, interesting. I do think that if the include subdirectory is going to live inside the configuration directory by default, then it would make sense for the default behaviour to be as I described, provided that's not too difficult to implement.

Also, just looking back at these flag names, I'm not sure using mechanism as a flag to point to the include directory is super intuitive. I tend to refer to the mechanism file (e.g. FACSIMILE or KPP) as "the mechanism" for the model, and I don't think that's just me. So I might expect the --mechanism flag to point to the location of the mechanism file. If the directory is going to be called include, then maybe the flag should just be --include or --include_dir? I think chemfile is fine for the mechanism path (my go-to would probably have been mechfile, but it's similar).

[rs028]

yes, this is the feedback I am looking for. I agree that "mechanism" makes one think of the .fac file, and also gives the impression that the fac file should be in the "mechanism" directory (which it can be, but doesn't have to). So I would also prefer to use the --mechanism flag to indicate the .fac file.
The "mechanism" directory contains the fortran files generated at compilation, including the pre-compiled shared library. In theory nothing inside it should be modified by the user (unless they also put the .fac file in there, but that is their choice). I used the term include as a placeholder, but I am open to other suggestions if people find it confusing. Another alternative is to re-introduce the term sharedlib_dir, but I am not sure it would be clearer.

Anyways, I can merge this as a temporary step, and we figure it out in the next PR.

[codecov]

Codecov Report

❌ Patch coverage is 8.00000% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 51.73%. Comparing base (c4ed6af) to head (9975471).
⚠️ Report is 31 commits behind head on master.

Files with missing lines	Patch %	Lines
src/inputFunctions.f90	0.00%	15 Missing ⚠️
src/argparse.f90	20.00%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #541      +/-   ##
==========================================
- Coverage   52.05%   51.73%   -0.32%     
==========================================
  Files          17       17              
  Lines        2096     2101       +5     
  Branches      166      167       +1     
==========================================
- Hits         1091     1087       -4     
- Misses        933      942       +9     
  Partials       72       72              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[rs028]

I have changed it so that the flags are now
--mechanism= chemical mechanism file (.fac)
--configuration= configuration directory
--shared_lib= autogenerated fortran files and shared library directory,
--mcm= MCM version

For the moment shared_lib is by default model/configuration/include but will change it after resolving #539. If there are no more comments I can merge, after approval.

PS: we can ignore codecov here, it only complains because the number of lines in the fortran files has changed.

[AlfredMayhew]

@rs028, I had a go at running this and it seems to work OK. One thing I noticed, and it might be intentional, is that the keywords are required for the arguments. You used to be able to just type something like ./build/build_atchem2.sh path/to/mech path/to/config etc., now you must type ./build/build_atchem2.sh mechanism=path/to/mech configuration=path/to/config. This isn't a huge deal, but does slow down the typing a bit!

[rs028]

@rs028, I had a go at running this and it seems to work OK. One thing I noticed, and it might be intentional, is that the keywords are required for the arguments. You used to be able to just type something like ./build/build_atchem2.sh path/to/mech path/to/config etc., now you must type ./build/build_atchem2.sh mechanism=path/to/mech configuration=path/to/config. This isn't a huge deal, but does slow down the typing a bit!

It does, although it makes scripts easier to manage and less error prone. It's the price we pay to use flags. Ofc, if people do not like it, we don't have to use the flag system!

BTW, it should be --mechanism= not mechanism=. Does it work without dashes too?

[AlfredMayhew]

Ok, I just wanted to make sure you were aware! Yes, I forgot the dashes in my GitHub comment, sorry about that! I used the dashes in my actual tests

[rs028]

Okay. From a user prospective do you think it is an overall improvement or should we hold off?

[AlfredMayhew]

I think this allows for more flexibility in how models are run, since you could have different models with the same mechanism sharing the same include directory (which you can't currently have since it needs to be inside the configuration subdir), and/or have multiple different mechanisms compiled at once (which you couldn't have originally, when include was in the base directory).

I do feel like the default behaviour I mentioned before when not specifying the shared-lib is potentially confusing and I would prefer that if a configuration directory is specified, then the default shared-lib path is PATH/TO/CONFIG/include/. Especially since include is supposed to be something that the user doesn't interact with much, someone may not realise that their model has built to the wrong directory.
I'm a little hazy on the long-term plan here, and how all of the different issues you've been linking intersect with each other, so if it makes sense for you to go ahead with this commit now, then that's fine by me.

[rs028]

I think this allows for more flexibility in how models are run, since you could have different models with the same mechanism sharing the same include directory (which you can't currently have since it needs to be inside the configuration subdir), and/or have multiple different mechanisms compiled at once (which you couldn't have originally, when include was in the base directory).

Yep, that is the idea.

I do feel like the default behaviour I mentioned before when not specifying the shared-lib is potentially confusing and I would prefer that if a configuration directory is specified, then the default shared-lib path is PATH/TO/CONFIG/include/. Especially since include is supposed to be something that the user doesn't interact with much, someone may not realise that their model has built to the wrong directory.

I agree. I am thinking of this as an intermediate step in that direction. I have to work on this in batches because of other commitments. I will open another PR to resolve these outstanding issues.

I'm a little hazy on the long-term plan here, and how all of the different issues you've been linking intersect with each other, so if it makes sense for you to go ahead with this commit now, then that's fine by me.

So my idea was to introduce a fourth argument to the build script so that the include directory can be specified indipendently from the configuration directory. This being a breaking change it makes sense to also make two other breaking changes at the same time: 1) repurposing the mcm argument which doesn't have a real use to specify the version of the MCM, 2) adopting the flag system. Now that this is in place we can go back to sort out how to deal with the include directory. I will put a note in open issue #539 about that. Hope this clear things up.