-
Notifications
You must be signed in to change notification settings - Fork 22
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
Dilute fix #408
Dilute fix #408
Conversation
…n environmentVariables.config is used to append a loss rate for each species to the mechanism.
This partially addresses the discussion on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I understand how this works. It makes sense - whatever we do, at some point if DILUTE
is set to non-zero then we're going to have to iterate over all species and apply the factor. So this mechanism seems reasonable.
A couple of comments:
(1) This seems like it will only work for a fixed value of DILUTE, but not for a constrained value. Is that an issue? I don't see a simple way to extend this mechanism to a constrained value.
(2) Currently this only checks for NOTUSED
. Could we also check for 0? If DILUTE
is 0
(also handle 0.0
and 0.0e0
etc by converting the string to real), then we don't want to bother adding all these extra rows, in just the same way as NOTUSED
. That will mean there's no overhead in the common can where no dilution is denoted by 0
. Thinking about it, we could go further and outlaw NOTUSED
as a valid value - insist that the input is 0
instead. This would require modification to the Fortran parsing of the environment variables, but simplify the possibilities for users.
Actually, the more I think about this, the more I think this could be changed to avoid the double call to
This removes the need to call |
At the moment
I agree. This is sensible. How hard is it to remove the
Does it make sense? |
Good point. I don't think it would be too hard to remove the AtChem2/src/inputFunctions.f90 Line 869 in a74ec61
But, that actually leads us on to thinking about what Currently, the whole process ends up with a value for To extend this to handle a constrained dilute, I think we can do something similar to what's done for the photolysis rates
etc, where
and then evaluate |
Oh, and in fact, rather then the RHS saying
|
Codecov Report
@@ Coverage Diff @@
## master #408 +/- ##
=======================================
Coverage 88.84% 88.84%
=======================================
Files 17 17
Lines 2259 2259
=======================================
Hits 2007 2007
Misses 252 252
Continue to review full report at Codecov.
|
Ok, please see #412 for what I believe is a working version. It may be helpful to manually grab that commit's contents and use it to replace the contents here (that way we easily keep all this conversation with the eventually merged PR). I have only briefly tested this, because I don't have any real testcases where
to reflect the new reactions created. But that line is not used anywhere, it's just cruft from the original file, so I don't see an issue. (In the case I used, the count of species is out by one anyway! - not sure why). |
Hi @spco, for the purpose of updating the manual, is this correct?
|
1 and 2 are correct, but |
Hi @bsn502 - you'll see that the (a) is possibly a lot of work, (b) means we lose the big test, (c) means Personally, maybe (b) is the right option for now - we can always manually reinstate |
Oh, and thanks for all your continuing work @bsn502 ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks okay to me.
@spco One question: how do we deal with acknowledgements? I think Beth name should be on this file. The copyright notice is probably not the right place. Previously I have added a small comment (see line 15 in tools/plot/plot-atchem2_v3.py
)
Sure, I think either solution would be fine - perhaps the acknowledgement line is the right way to go? |
Probably best to |
@bsn502 can you add an acknowledgement line to the header similar to line 15 in |
…s the most up to date version.
Is this ready to be merged? @spco does it need a rebase? |
Looks like it to me. I would squash and merge using the GitHub GUI, given the slightly circuitous commit history to what is essentially changes to one file and the removal of a directory. |
New files: ./dilute.py
Modified files: ./build/build_atchem2.sh
The new code uses the "DILUTE" number in environmentVariables.config to amend the mechanism provided. Tests have been successful and the code is very useful, especially when using a larger mechanism file.
Current issues: the model needs to be re-compiled if changes are made to the dilution rate; the changes currently only work if model folder is called "model".
An example using the ethane subset from the mcm and an initial concentration of ethane shows how its concentration changes under three different DILUTE rates.