Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend tests to cover more .fac file examples #47

Open
spco opened this issue Apr 5, 2017 · 5 comments
Open

Extend tests to cover more .fac file examples #47

spco opened this issue Apr 5, 2017 · 5 comments
Labels
Milestone

Comments

@spco
Copy link
Collaborator

spco commented Apr 5, 2017

No description provided.

@rs028
Copy link
Collaborator

rs028 commented May 31, 2017

I am thinking about rationalizing and extending the tests. At the moment they are a bit similar to each other and often test only specific parts of the code (sometimes very specific, for example single_reac_of_interest).
I think each test may be used to test a few (unrelated, if possible) settings and/or initial conditions or outputs at the same time. Thoughts?

@spco
Copy link
Collaborator Author

spco commented Jun 1, 2017

Yes, I totally agree. We should attempt to reduce the overlap between tests, although I wouldn't push that too far, in case we start to lose previous test coverage.

Looking at the tests we have in place, I thinkshort_extended and single_reac_of_interest should be tweaked. The particular place we should be trying to cover is environmentVariables.config, which has lots of options, but the current tests all largely use the same choices.

As a first draft of changes to make, could I suggest:
(1) short_extended to use fixed RH, with H2O = CALC.
(2) single_reac_of_interest to use DEC = CALC, ROOFOPEN = 0
(3) leave most of the spec... tests as they are (don't want to change too many variables between tests) except perhaps change spec_yes_env_yes and spec_no_env_yes1 to use a more helpful constrained variable - since BLHEIGHT is never used in any calculations, constraining it doesn't have much effect on the solution!

What do you think of those suggestions? Happy to talk it through more, that's just an initial suggestion.

P.S. as I've touched on in (3) above, none of the source code itself makes use of either blh or dilute. Taking a look at the .fac files, I don't see them using it either. Is it worth keeping these around in case of a hand-crafted .fac file that uses them? Or is there an option in the MCM to use these? I guess my point is that full.fac should contain just about all the reactions, and none use either of these. Happy to leave them in there if there is a possibility of them being used - otherwise they are just clutter!

@rs028
Copy link
Collaborator

rs028 commented Jun 1, 2017

Sounds good. This is what I was thinking:

  1. Test the single reaction of interest in short and get rid of that test
  2. have a few short_extended tests to check all the environmentVariables.config options
  3. the spec_* tests as they are now, with some tweaks
  4. a full test with most options turned on

BLHEIGHT and DILUTE are meant to be used if you want to add processes to the base MCM model. I can add at least one of them in the full test.

What do you think?

@spco
Copy link
Collaborator Author

spco commented Jun 1, 2017

  1. single_reac_of_interest was created to check that behaviour seen in Reorganization of configuration files #34 and Missing output given single species of interest #81. I'd agree that this is fixed and unlikely to return, so removing the test is fine.
  2. Yes, perhaps variations of config options while using the same basic MCM selection is a good way to organise it.
  3. Yes
  4. Yes - I like the idea of adding a BLHEIGHT and DILUTE dependence to the full test. Try placing them into a fairly sensitive/significant reaction, which should highlight if there is any issue. Perhaps have BLHEIGHT fixed and DILUTE constrained, or vice versa?

@rs028
Copy link
Collaborator

rs028 commented Jun 1, 2017

Okay, I am happy to take this over, as it will give me a good feeling of the status and performance of the model.

BLHEIGHT is easier to add than DILUTE, because the latter is used for very specific simulations (eg, an environmental chamber).

Adding reactions and processes to the base model it's very common, and it relates to #17 (maybe that is too complicated to implement, but ideally there should be a procedure to add environment constraints, eg, instruction on which parts of the code need to be modified, if that makes sense)

@rs028 rs028 mentioned this issue Jun 2, 2017
@spco spco removed this from the Pre-function parser milestone Jun 16, 2017
@rs028 rs028 mentioned this issue Apr 18, 2018
@rs028 rs028 removed their assignment Feb 11, 2019
@rs028 rs028 added this to the version 1.x milestone May 15, 2020
@rs028 rs028 moved this to Testing and Numerical Issues in Roadmap Sep 22, 2022
@rs028 rs028 added this to Roadmap Sep 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Testing and Numerical Issues
Development

No branches or pull requests

2 participants