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

New obthermo tool with corresponding data #200

Merged
merged 16 commits into from Jul 29, 2015
Merged

New obthermo tool with corresponding data #200

merged 16 commits into from Jul 29, 2015

Conversation

dspoel
Copy link
Contributor

@dspoel dspoel commented Jul 15, 2015

This provides a new tool that extracts thermochemistry (Delta H formation etc.) from a Gaussian log file. The patch also provides the necessary reference data from a large set of calculations and experiments.

@dspoel dspoel changed the title New obthermo tool with correspoding data New obthermo tool with corresponding data Jul 16, 2015
@ghutchis
Copy link
Member

Dunno, there seems to be an issue with appveyor. Can you try "bumping" this with a new update?

@ghutchis
Copy link
Member

It looks OK to me, although Windows builds use a very different compiler/linker.

@dspoel
Copy link
Contributor Author

dspoel commented Jul 23, 2015

Could it be that the file data_utilities.cpp is not included in the build on Windows? It is in the CMakeLists.txt file.

@baoilleach
Copy link
Member

The problem is that the functions in data_utilities are not exposed from the openbabel DLL. On Linux, by default, everything is exposed, while on Windows you have to specify which ones to expose.

More to the point, we tend to avoid extending the OB API unless necessary. I'd suggest you simply move the data_utilities.cpp out of the openbabel core and into the thermo target. Obviously there's no linking problem then.

However, if you can make a case for keeping them where they are (e.g. are these functions of general use to OB users?), I'm happy to do the necessary to expose the functions.

@dspoel
Copy link
Contributor Author

dspoel commented Jul 24, 2015

Thanks for your comments. One of the new routines energyToKcal(std::string) is used by gaussformat.cpp to convert units to kcal, this in order to remove the shattered physical constants in the code. I can remove the call and put the constants back if you think that is better.

@dspoel
Copy link
Contributor Author

dspoel commented Jul 25, 2015

In fact I would like to use the function extract_thermochemistry from my GROMACS programs, this is why I implemented all this, so I would be interested in an API change. If there are better ways I am open for suggestions though.

@baoilleach
Copy link
Member

Sorry it's taking me a while to respond.

Could energyToKcal be implemented instead of a set of consts in the header file (e.g. "const double JOULE_TO_CAL = whatever")? This wouldn't need any API changes though we could expose it if useful. We have something similar for DEG_TO_RAD I think. The nice thing about a const is that the IDE will fill in the name of the variable, though I guess a function taking an enum would do the same.

I'll do the necessary work for extract_thermochemistry tomorrow. Hopefully I can send you a pull request directly, and then you get to hassle me about my patch! :-)

@baoilleach
Copy link
Member

Can you email me a test file and associated command-line. I've tried a few files, but nothing I have seems to trigger the code.

@dspoel
Copy link
Contributor Author

dspoel commented Jul 29, 2015

On 28/07/15 17:20, baoilleach wrote:

Can you email me a test file and associated command-line. I've tried a
few files, but nothing I have seems to trigger the code.


Reply to this email directly or view it on GitHub
#200 (comment).

obtherm butane-g4.log.gz
should do the trick.

David van der Spoel, Ph.D., Professor of Biology
Dept. of Cell & Molec. Biol., Uppsala University.
Box 596, 75124 Uppsala, Sweden. Phone: +46184714205.
spoel@xray.bmc.uu.se http://folding.bmc.uu.se

@dspoel
Copy link
Contributor Author

dspoel commented Jul 29, 2015

…t though for initialising heat of formation table, but replaced it with more efficient lookup. Add extract_thermochemistry to the public DLL API and namespace it in Open Babel.
@baoilleach
Copy link
Member

Thanks - you should have received a pull request for my code. If you accept this, I presume it will magically appear here.

baoilleach added a commit that referenced this pull request Jul 29, 2015
New obthermo tool with corresponding data
@baoilleach baoilleach merged commit c564a64 into openbabel:master Jul 29, 2015
@dspoel
Copy link
Contributor Author

dspoel commented Jul 29, 2015

Cool, thanks! We're about to submit the paper on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants