-
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
Shared library #370
Shared library #370
Conversation
Codecov Report
@@ Coverage Diff @@
## master #370 +/- ##
==========================================
- Coverage 90.42% 90.28% -0.14%
==========================================
Files 15 15
Lines 2109 2121 +12
==========================================
+ Hits 1907 1915 +8
- Misses 202 206 +4
Continue to review full report at Codecov.
|
@rs028 I think this is ready for review. The only failures are from the code coverage, which I don't think I can do much about. Work remaining: Update the README (and the wiki, once v1.1 docs are out) with the changes to how things run. In particular, I'd value you thoughts on whether the new shared library should normally go into |
Good question. I tend to put the With regard to |
This feeds into a wider discussion of how flexible we want to be, versus how easy we want it to be to not go wrong. Currently I'd be interested in your thoughts on this. Of course, if we get round to moving to a smarter command line interface, where we accept named arguments rather than just positional ones, then that might make it easier to offer a simple hard-to-break default interface, with more powerful options available if users really want to use them - but make it far easier to just plump for an easy setup. As an example, I'm thinking In extremis, users also have access to more granular arguments specifying e.g. environment constraints come from some other location. In essence, I want it to be easy to just point at the root directories for whole models, and a bit harder to specify more granular details - that way, you nudge users to do sane things like storing all the data for one model in the same location, and storing the output of the model in the same location. |
Back to the matter in hand - yes, we can force the location of the shared library to that of the mechanism file, and I think that's sane behaviour (in fact the only reason this doesn't do that currently is because that wouldn't work for I can see an argument for the default being |
This all sounds very sensible. Having a lot of arguments is a bit of a pain, but also can cause to errors. With or without a smarter command line interface, it may be a good idea to simplify and nudge towards good behaviour. Possibily the easiest solution is to decide that the directory This is probably a different PR? Or needs to be resolved now to finalize this one? |
I'd suggest if you're happy for now, we merge this PR and open a new one. I might even have a look at improving the interface. Are you happy if this PR goes ahead? |
Yes, okay. |
Hi @rs028 - I'm back from leave so looking to wrap this up. Are you happy if I rebase this and merge into 1.2-dev (which may want to be 2.0-dev in the end)? I can also open the other PR for the restructuring as discussed above. |
Welcome back :) Yes, I am happy to do so. |
…hich is a sample file. This is only temporary, and will be replaced by a generated file.
…2 as an input argument to update_p(). Update mech_converter.py to autogenerate the head and tail of the new file.
…rently still uses src/gen/mechanism-rate-coefficients.f90 as the source file for the shared library.
…ct interface' in tools/fix_indent.py.
…IC flag on the main executable.
…ivalent for the tests) and load that at runtime.
… that for the travis build test.
…or mechanism.f90 to go to.
…cutable constantly being rebuilt. This may not be sufficient if the target of sharelib (denoted by SHAREDLIBDIR) changes - need to investigate.
…ilation parameter to ignore unused dummy arguments in mechanism.f90.
…e default for setup_atchem2.py to point to model/configuration, meaning we can also remove the optional arguments we were having to add in .travis.yml.
@rs028 Before I commit this, I want to update (a) the README.md to reflect the change in the build process. I will also want to update the documentation. Should I update the contents of |
…ce the build script on Travis, and fix a comment.
Good question. My plan was to convert the files in This would make the wiki redundant, as it would be a bit cumbersome to keep it in sync with the latex files, but I am not sure what the best approach is. Right now I have changed the |
Ok, yes. I guess we should deprecate the wiki (either remove it, or add bold headers explaining that it's deprecated) in order to not confuse users. https://github.com/palantir/godel/wiki/GitHub-wiki seems to be a useful tool (it updates the wiki to reflect the contents of Perhaps the wiki could also be updated only at the time of release, like the LaTeX? Would that be useful to users? |
I am leaning towards removing it once the transition to latex is complete. Maybe keep one or two wiki pages with practical examples of usage that won't fit in the manual. What do you think? |
Perhaps that's a sensible compromise - let's go with that. Point users to the main docs, but also supply some useful extras that may not be ideally suited within the main docs. |
…m-rate-coefficients.f90 from travis/run_style_test.sh as it's never generated now.
Addresses #369.
Convert the .fac file to a standalone Fortran module, which is dynamically linked as a .so to the main AtChem2 executable.