-
Notifications
You must be signed in to change notification settings - Fork 163
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
libnwchem patch (compile as shared) #13
base: master
Are you sure you want to change the base?
Conversation
The imaginary frequency polarizability code was removed (or should be if I’m remembering wrong) because it was never validated sufficiently and there was reason to believe it was not 100% correct.
Sorry. I wish it was not this way but nothing changed in 5+ years and I needed closure.
|
@ebylaska Is the heaviest user of NWChem's Python capability (AFAIK) so he should review those changes. |
A couple of comments/questions
|
I didn't change any of the code used by geninterface or the python module, but I haven't tested whether they work after my changes either. This code accesses the rtdb from python-ctypes and pushes strings directly to the input rather than relying on a temporary ".nw" file. As for NWCHEM_MODULES, I left out modules that didn't compile on my system, but again the only actual changes to the source code were adding push_inp_cstring, push_inp_string, and pop_inp_string. The python files are new and existing python files were not changed. libnwchem.F is not included in normal builds. These worked in version 6.5, but I would appreciate it if someone could clear up "push_inp_cstring.c" since I am not an expert in C to Fortran calling conventions for strings. libnwchem.so is loaded by python-ctypes in nwproto.py, and a list of wrapped functions are defined in nwchem.py:54 toplev = ["geom", "bsse", "bas", ... The rest of the interaction happens by pushing strings to input and calling those top-level functions from the shared library. |
I just ran the included test.py script and found I needed to remove string passing to nwchem_init, since fortran didn't seem to be reading them correctly (even at fixed size). Also, to ensure that all symbols are included, I added a call_all function to libnwchem.F that tricks the compiler into recursively including all needed functions. Finally, older versions of openmpi will fail on dll load with "unable to open (mpi lib)" errors. This is a problem with openmpi itself [https://github.com/open-mpi/ompi/issues/3705].
so with this change, the test works on my system. |
Hi David,
Passing strings between Fortran and C is a bit messy because in Fortran a string contains its length whereas in C it is simply a fixed size piece of memory and the end of the string is handled with the \0 character. Nevertheless, doing these kinds of things has been made a lot easier with the ISO_C_BINDING mechanisms. An example of calling C from Fortran can be found in util_nwchem_srcdir.F and utilc_nwchem_srcdir.c. However, I think there is enough of an example there to be able to work out how to call Fortran from C.
Best wishes,
Huub
…________________________________
Hubertus van Dam
Brookhaven National Laboratory
Mon, Fri: ISB 114 (Bldg 734)
Tue–Thu: CSI 2-165 (Bldg 725)
Tel: 631-344-6020
[email protected]<mailto:[email protected]>
www.bnl.gov/compsci/people/staff.php?q=139<https://www.bnl.gov/compsci/people/staff.php?q=139>
www.linkedin.com/in/HuubVanDam<http://www.linkedin.com/in/HuubVanDam>
orcid.org/0000-0002-0876-3294<http://www.orcid.org/0000-0002-0876-3294>
From: "David M. Rogers" <[email protected]<mailto:[email protected]>>
Reply-To: nwchemgit/nwchem <[email protected]<mailto:[email protected]>>
Date: Thursday, February 1, 2018 at 8:30 PM
To: nwchemgit/nwchem <[email protected]<mailto:[email protected]>>
Cc: Subscribed <[email protected]<mailto:[email protected]>>
Subject: Re: [nwchemgit/nwchem] libnwchem patch (compile as shared) (#13)
I just ran the included test.py script and found I needed to remove string passing to nwchem_init, since fortran didn't seem to be reading them correctly (even at fixed size). Also, to ensure that all symbols are included, I added a call_all function to libnwchem.F that tricks the compiler into recursively including all needed functions.
Finally, older versions of openmpi will fail on dll load with "unable to open (mpi lib)" errors. This is a problem with openmpi itself [open-mpi/ompi#3705].
[jack:26853] mca: base: component_find: unable to open /usr/lib/openmpi/lib/openmpi/mca_paffinity_hwloc: perhaps a missing symbol, or compiled for a different version of Open MPI? (ignored)
[jack:26853] mca: base: component_find: unable to open /usr/lib/openmpi/lib/openmpi/mca_carto_auto_detect: perhaps a missing symbol, or compiled for a different version of Open MPI? (ignored)
[jack:26853] mca: base: component_find: unable to open /usr/lib/openmpi/lib/openmpi/mca_carto_file: perhaps a missing symbol, or compiled for a different version of Open MPI? (ignored)
so with this change, the test works on my system.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub<#13 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AEcvnlmjAyGmPfYDJY43B50BKRL6AR_Bks5tQmUigaJpZM4RzcsE>.
|
If we are using ISO_C_BINDING now, I’ll write a tutorial. I have a lot of examples from other projects.
|
I'm OK with how it works now, but string passing would make it better (allowing the caller to set input names and memory sizes using strings). If someone wants to do this, it would require:
I'm more worried about another place in the code where C passes a string to Fortran, in the newly added |
790a699
to
4524b64
Compare
This patch adds 3 features:
It's much more flexible to be able to open nwchem from python as opposed to the other way around. While the enforcement of types has gotten better in the present version of nwchem, it prevented me from adding fake calls to the major functions, so the linking system is currently leaving out important functions from libnwchem.so. Also, compiling libnwchem.so on Linux requires replacing all compilers with a script that places -fPIC on the options list. Even so, a few errors are thrown because of the "-Wl,--no-undefined" link option:
These prompted removal of "task_jefftce.o task_ncc.o" from task/GNUmakefile. It looks like they are not presently used (but I wouldn't mind having imaginary freq. polarizability calcs though).
Compiles (after much chagrin) on Ubuntu Trusty (14.04.2) using: