-
Notifications
You must be signed in to change notification settings - Fork 23
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
Specifying Custom FORTRAN Functions in the Mechanism #507
Conversation
@AlfredMayhew Thank you for this. It looks like a really good addition to the model. I need some time to understand the changes a think how to better integrate them, but it's definitely something useful! |
Of course, no worries. Let me know if there's anything I can do to help. |
@AlfredMayhew in order to review this PR could you please first update to the lastet version in the master branch? Thanks |
I think I've done this now? Is it just the master branch that needs updating? I can't seem to sync the custom_functions branch easily because of conflicts. If you need more then just let me know. |
I can't see if you done it. The usual procedure for me is:
When you rebase it will ask you to resolve the conflicts (if any). This will bring the branch even with the current master branch and apply your changes on top. |
729138c
to
279d30b
Compare
Thanks, I think I've done that now. I was a bit confused about resolving conflicts but I think I've done it now. I've run a test and it still seems to be working as it was but now GitHub is showing no conflicts. Let me know if I've missed anything! |
I am guessing that the tests fail because the makefile is looking for |
Two more things.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #507 +/- ##
=======================================
Coverage 52.05% 52.05%
=======================================
Files 17 17
Lines 2096 2096
Branches 166 166
=======================================
Hits 1091 1091
Misses 933 933
Partials 72 72 ☔ View full report in Codecov by Sentry. |
A better way to add a test (instead of using the function to multiply two numbers as I suggested above) is to use |
I have added a test replacing KMT15. As with my latest post to #514, this test currently fails because the produced output is slightly different from the output of spec_model_1. Here, the biggest difference is for C2H4 of 1000 molecules cm-3, which is of course very small. I'm also reassured that this difference doesn't compound. The timestep after a difference between the two models shows no difference between the two models. If you're happy then I can update the test to use my model output. I'll also add some text to the manual at some point too. |
This looks good to me. I suggest we first merge PR #514 (you should be able to, let me know if you aren't), then rebase this branch. |
3cbbb58
to
9b99846
Compare
I have updated this branch to work with the new stoichiometry branch. Let me know if there are any other changes you think would be beneficial. |
2353f38
to
745a333
Compare
I have added a couple more comments. This needs to be rebased again after PR #515 has been merged. |
…in the mechanism file.
745a333
to
53e209d
Compare
Ok, that should be those changes made, including the rebase. I can see a grayed out merge pull request button, so I should be able to merge this once you approve it. Let me know if there's anything else I need to change! |
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 good to me. The documentation will go with the manual revision I am (slowly) working on. Unless @spco has further comments I am happy for this to be merged.
Hi,
There's been a few times where I have felt it would be useful to be able to represent rates in the mechanism as a function with differing variables for different reactions. Previously, I have done the function bit beforehand and just included very long rate expressions in my mechanism files (with all of the calculation steps expanded into one line).
I have made some changes that I believe will allow a user to define custom functions. If this is something that you think would be valuable to add to AtChem2 then I'm happy to make any further changes and write a little in the documentation to explain this. Hopefully this wouldn't change the user experience for the majority of users (the
customRateFuncs.f90
file can be left as its default of an empty module if it isn't needed), but adds some quite powerful flexibility for people wanting to use more complex rate expressions.I'll try and outline the changes below. Please let me know if there's any way I can improve the implementation.
model/configuration/customRateFuncs.f90
. This would be where users can specify their functions withing thecustom_functions_mod
module.build/mech_converter.py
that looks for "function function_name (
" and then addsfunction_name
to the list of reserved names. I think that this bit could be improved. Do you know another way of parsingcustomRateFuncs.f90
to look for function names? I'm also not sure if I should be including subroutines here too? I don't really know FORTRAN very well, so I'm not 100% sure of the difference between functions and subroutines.customRateFuncs.f90
to the makefile skeleton so the build stage works properly.I have tested this with a basic function that multiplies numbers together, so that my
customRateFuncs.f90
file looks like this:I can then include the following reaction in a mechanism file:
% multiplyNumbers(2.0, 0.5) : = A ;
. The model runs fine and A seems to be produced at the correct rate! I've also tested inputtingTEMP
into this function, which seems to work (provided I change the type of inNum2). Being able to input the environment variables into these functions adds even more flexibility.Again, let me know if you can think of any ways to improve this and if you think it would be suitable for incorporation into the model.
Thanks,
Alfie