Minor issues and questions

[rs028]

Here are a list of various miscellaneous issues and questions that should be addressed, fixed or documented. They are in no particular order and most of them are fairly low priority. Some of these are features that just need to be documented in the wiki.

1. check that initial and and final model time are within the initial and final constraint timestamps before starting interpolation (see Interpolation of data is wrong when asked for a time before the first provided time #294 for this issue).

2. get rid of the makefile.local files.

3. make sure that H2O and RH are mutually exclusive (if one is constrained or constant the other must be calculated).

4. RO2list and photolysisRates.config are specific to one version of the MCM. It should be easy to swap with other versions. [ addressed in PR Update MCM related files #290]

5. make sure that the text in environmentVariables.config is unequivocal and intuitive (eg ROOFOPEN=ON is a bit confusing, ROOF=ON is better). Simplify where possibile (eg, JFAC=1 and JFAC=NOTUSED are the same). What happens if there is a typo (eg, "CONTRAINED" instead of "CONSTRAINED")?

6. the naming of the variables in the user interface (ie, python tools, filenames of the constraints, name of variables in the configuration files) and in the code (ie, the f90 files) should be consistent. This was in part discussed in the beginning (eg consistent naming of Environment Variables #83) but we never really resolved. Should the name be the same in the user interface and in the code, or is it better to use different naming conventions? For example, at the moment its BLHEIGHT in the code, and in the config files, and BLH in the python tools and filenames of the constraints. (solved by Consistent env var names #354)

7. is there a problem in mechanism.reac? The header of the file seems to create problems when productionRatesOutput.config and lossRatesOutput.config don't have the same content and/or just one or no variables. [unclear] (solved by Unify rates output config files #321 and Add a header to mechanism.prod to match that of mechanism.reac. #325?)

8. is interpolation of the constraints done before or during the integration? Is it ignored when there are no constraints or does it create an overhead?

9. is mechanism-rate-definitions.f90 used at all?

10. what is constrainedFixedSpecies.config for?

11. what happens if a species or a photolysis rate appears twice (by mistake) in constrainedPhotoRates.config, constrainedPhotoRates.config, constrainedSpecies.config?

12. is errors.output useful? most of the error messages seem to be printed to screen (i.e., redirected to the log file by the HPC system). [moved to Improve output files #320]

13. is it worth upgrading the Python scripts to Python3? Or making them indipendent on the Python version present on the system?

14. What is initialConditionsSetting.output and what does it mean? It can get really big. [moved to Improve output files #320]

[spco]

Makefile.local is solved by #285

[spco]

constrainedFixedSpecies.config is for constant-value species. Its name will become speciesConstants.config or speciesFixed.config in the Great Reorganisation.

[spco]

If you mean mechanism-rate-declarations.f90, then yes, it's used in mechanism_rates() in solverFunctions.f90.

[spco]

On 1., the current behaviour in getConstrainedQuantAtT() is this: if the required time is within the time covered by the constraint data, then it interpolates. Otherwise, it returns the value of the last constraint data.

This certainly needs fixing in at least one case: if the required time is before the first constraint data time, then the last constraint data value is still returned. This is obviously not what is wanted.

I've opened #294 for this - see discussion there.

[spco]

On 8., both photolysis rates and environment variables are only evaluated when needed - each is interpolated individually, only when constrained. This happens each time mechanism_rates() is called from FCVFUN(), so the number of times that happens is controlled by CVODE as it completes the integration.

Species interpolation is very similar - it is called once for each of the constrained species in FCVFUN(), plus once when setting the initial conditions of each of the constrained species.

At this time I don't see a way to reduce the cost of this - once I've profiled more extensively, that will shine a light on how expensive this all is, and whether it's worth looking harder at.

[rs028]

Thanks for clarifying. About points 1 and 8 they are clearly not minor points, and there needs to be more investigation, so I cross them out from here and move the discussion to #294.

About constrainedFixedSpecies.config it means that species can also be set to constant value, like the photolysis rates, which is not documented, So I will just make a note to add that to the wiki.

[rs028]

@spco I added this to the Milestone for version 1.1 because there are only a few points left and some of them are just questions to improve the documentation. If some require more time than you have at the moment, they can be moved to their own issue.

[spco]

On number 11 above - it shouldn't matter if the same species is mentioned twice in the same file of either speciesConstant.config, speciesConstrained.config, photolysisConstrained.config. It's inefficient, as it'll all be read in and stored and processed, but it shouldn't matter to the result. But it looks like we don't check that there's no conflict between species defined as constant or variably constrained? I'll make this a separate issue.

[spco]

On 3. above, currently we process RH, and then H2O. RH can be supplied as a constrained, or fixed value. H2O is then either calculated from RH (via convertRHtoH2O()), or set as constrained, set as fixed, or given a default value. There are 2 issues with this setup currently:

1. there's nothing to stop the user setting both to fixed or constrained values that are incompatible.
2. it's a one-way relationship, because H2O can never be used to calculate RH.

The setting of environment variables is one of the parts of this code that is a bit wonky in general. Ideally it wants a bit of an overhaul and rethinking to ensure that the user has decent flexibility in what they can supply as inputs, but cannot do things which are incompatible. We also already have a future plan to allow users to possibly add new environment variables themselves, so I'd suggest that we bundle all of these up into one work package in v2, rather than try to do much here. What do you think @rs028?

[spco]

On 6. above, we have the following naming:

environmentVariables.config: 'PRESS', 'TEMP', 'RH', 'BLHEIGHT', 'DILUTE', 'ROOFOPEN', 'M', 'H2O', 'DEC', 'JFAC'
mechanism_rates:             pressure, temp,   RH,   BLH,        DILUTE,   ROOFOPEN,   M,   H2O,   DEC,   JFAC

so the differences are in pressure and blheight. Looking to a future where we might be able to implement new enviroment variables, the only sensible option is that the config and the code have to talk in the same language, otherwise new environment variables would need a shorthand name for the code or something like that - messy.
To maintain compatibility with existing config files, I'd suggest that we always use 'PRESS' (or lowercase) and 'BLHEIGHT' (or lowercase). There are only a few places where this needs changing, so I'll raise a PR (#354) for that. Let me know if you have any objection.

[spco]

Re the above on point 6, current the python mech_converter.py uses BLH as a reserved keyword, but I think it's fine to just replace this with BLHEIGHT.

[spco]

Re 13. above, I'd keep this for v2, but ideally this should be possible to make the scripts version-independent, as they don't have too many dependencies. If it's not possible, I'd recommend upgrading to python 3 in AtChem2v2.

[rs028]

Okay, so #354 solves 6 and 13 can be made into a separate (low priority issue).
That leaves 3 and 5. Do you want to do those now or leave for future versions?

[spco]

I would suggest we leave those both for the future - both are embroiled in the environment variables section, and should be treated together when we have a plan for that.

[spco]

And as such, I think our v1.1 milestones have all been completed?

[rs028]

yes, I think we are good to go for v1.1. Unless there is something else you have time to do this week.

[spco]

I think my time here is finished for a while, so it's as good a time as any to release v1.1 from my perspective!