Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #535 +/- ##
==========================================
- Coverage 52.05% 51.75% -0.30%
==========================================
Files 17 17
Lines 2096 2104 +8
Branches 166 167 +1
==========================================
- Hits 1091 1089 -2
- Misses 933 943 +10
Partials 72 72 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
The auto-generated mechanism files ( The The |
|
@spco @AlfredMayhew please have a look at this. It should resolve PR AtChem/AtChem-tools#4, following Sam's suggestion. The documentation will have to be changed, and I don't have time right now, but I can merge this PR and change the manual later, so Alfie can proceed with the modifications to the Atchem-tools in the meantime. |
|
Thanks @rs028. I've tried to have a go at using this and although the model is running, I'm still seeing the mechanism files in the configuration directory. I'm sure I'm doing something wrong, so let me know and I'll try again with the testing. Here is what I did: First, I cloned your git repo and checked out the Then, I copied the Makefile template and adjusted the paths for CVODELIBDIR, OPENLIBMDIR, FRUITDIR. Then I compiled and ran the default model: Looking at the contents of the configuration directory then shows files such as Also, one additional issue I found was that the default behaviour of the build script (without specifying a configuration directory), e.g. passing Am I doing something wrong? |
|
Thanks for that. Teaches me to not submit changes in a hurry :) |
|
so there is a little issue here, right now the build script takes 3 arguments:
if we add model/include then we have two options. either we add fourth argument (which is a breaking change), or we put Any thoughts? If @bsn502 and @willdrysdale also want to pitch in, it would be good to have a wider view of the user's preferences. |
|
I don't mind changing the arguments as it's easy enough to adjust my python tools, and I run 99% of my models through python anyway, but I understand that it's perhaps less convenient for others. |
|
I agree. The original logic was that a user should be able to point at a specific configuration, a specific set of constraints, and a specific mechanism, so one could run models with the same configuration and different mechanisms, or viceversa. The problem is that it was implemented somewhat inconsistently, as we are finding out. The purpose of this PR is resolve some of this issues, but I see this as an intemediate step, hence I am reluctant to change the user interface without a complete solution. I think if we put |
|
I've tried to catch myself up on this issue, but apologies if I have missed something as this is deeper than I have previously looked at the model. If I understand correctly:
@bsn502 can definitely give a better option from a user perspective, but I have a couple of things docker related:
|
|
Hi Will, sort of. Part of the this PR is fixing a bug where the model was always recompiling the shared library, even if the mechanism file had not changed. It also hides one of the executable flags ( |
|
@AlfredMayhew when you have time to review and approve this, I can merge and we can continue the discussion on #539. |
|
@rs028 Sure, I should have some time this afternoon! @willdrysdale, I think Sam has mentioned the containerised version before (or maybe it was you another time?), so it's definitely something I should look into. I'm currently not really familiar with how any of that works (same with docker / apptainer), so it would be something I'd have to do some work to implement. I guess for now we can run with this fix and we can switch across later if it's worthwhile (either when I find time or if someone else wants to make those changes to the tools directory themselves). |
|
@rs028 Ok, I've had a quick test and it seems to be behaving as expected now (creating files in First, I'm running into some issues using This is not an issue for the test case I wrote in Secondly, annoyingly, this still doesn't seem to resolve my issues when running parallel models! Again, this may be best for a separate discussion, or to just switch across to the containerised version, but I'll briefly describe the issue here. Again, this seems to relate to Then, when the model is run, there is a memory allocation error: Thanks! |
|
It may be my mistake. In the Makefile we have: Try changing |
|
Nice, that change seems to have solved both problems! |
|
Great, thanks. These lines refer to the compilation of the shared library (hence |
Solves a couple of issues with the shared library.
One is described in #414: the build script uses the same argument as destination for the fortran mechanism files and for the shared library, even though (theoretically) the shared library can be elsewhere (as set by the
$SHAREDLIBDIRvariable in the Makefile). This makes the--shared_libflag for the executable redundant. I think it makes sense for the shared library to be always in the same folder as the other mechanism files, especially now that we have acustomRateFuncs.f90in the same directory. With this PR we forcemechanism.soto always be in the configuration directory and remove the associated flag from the executable. The underlying structure remains in place, so we can always reintroduce the flag in the future, if we change our minds.The second issue is related to the conversion of the
.facfile. The build script always runs the python converter, which means that the fortran mechanism files are always updated, even if they are not changed. This defies the purpose of having a shared library, which should only be recompiled if the.facfile is changed. To correct this, the execution of the python script is moved from the build script to the Makefile. The build script is simplified but the user interface does not change.Also introduces some error handling and more comments for both the Makefile and the build script.