Allow coordgen to run without maeparser.#55
Conversation
d-b-w
requested review from
JarrettSJohnson,
ZontaNicola,
greglandrum and
ricrogz
d-b-w
force-pushed
the
builtin_templates
branch
from
4f03dfa to
528160b
Compare
ricrogz
left a comment
ricrogz
left a comment
There was a problem hiding this comment.
I had a quick look over this, mentioned a few things I have seen.
d-b-w
force-pushed
the
builtin_templates
branch
2 times, most recently
from
fdbc04f to
1b75d28
Compare
greglandrum
left a comment
greglandrum
left a comment
There was a problem hiding this comment.
A few suggestions and questions here
| _MOLECULE = """ | ||
| {{ | ||
| auto molecule = new sketcherMinimizerMolecule(); | ||
| std::array<std::tuple<int, float, float>, {atom_total}> atoms = {{{{ |
There was a problem hiding this comment.
Given that this is just a template in a code-generation function, I don't think it's critical, but the duplication of the name atoms here makes my head hurt.
I'd rename this variable to atomVect
| std::array<std::tuple<int, float, float>, {atom_total}> atoms = {{{{ | ||
| {atoms} | ||
| }}}}; | ||
| std::array<std::array<int, 3>, {bond_total}> bonds = {{{{ |
There was a problem hiding this comment.
I'd rename this variable to bondVect
| @@ -0,0 +1,23 @@ | |||
|
|
|||
There was a problem hiding this comment.
Does this file need to be added to the install list in CMakeLists.txt?
It looks like all the other header files are there, but I'm not sure all of them are actually needed (that'd be another PR to fix).
There was a problem hiding this comment.
I totally didn't even think about that. But I don't think so, because the function that CoordgenTemplates.h has hidden visibility, so a client can't use the function.
| @@ -0,0 +1,54 @@ | |||
| #pragma once | |||
There was a problem hiding this comment.
Does this file need to be added to the install list in CMakeLists.txt?
It looks like all the other header files are there, but I'm not sure all of them are actually needed (that'd be another PR to fix).
There was a problem hiding this comment.
I think that this is also private. We could expose it, though.
There was a problem hiding this comment.
Na, I think it makes sense to do a separate PR to only install the bits that are actually required to use the library.
I was just pointing it out here to be consistent; definitely not required
greglandrum
left a comment
greglandrum
left a comment
There was a problem hiding this comment.
LGTM
Do you think it's worth me trying an RDKit build against this before we merge it? I'm assuming it's not going to have an impact there aside from making it simpler to build the RDKit wrappers
ricrogz
left a comment
ricrogz
left a comment
There was a problem hiding this comment.
I noticed a problem in the CMake file, and some outdated documentation.
| filename = getUserTemplateFileName(); | ||
| loadTemplate(filename, m_templates.getTemplates()); |
There was a problem hiding this comment.
Some silly thought for maybe another PR: if we move this a few lines up, before m_templates.getTemplates() = coordgen_templates();, do we get to overriding the default templates for free?
There was a problem hiding this comment.
I didn't want to change user-visible behavior in this PR.
I agree, though, it might make sense for user templates to hit before the default templates.
|
Greg - I've been using this in my local RDKit and Schrodinger core suite, I think it's OK. |
Now templates are always bundled into the coordgen library,
so there is no search on the file system. coordgen can also
be built without maeparser support (USE_MAEPARSER=OFF). This
disables user-local template files, but it allows coordgen
to be built without reliance on the maeparser library
(and therefore without boost).
This is useful for a couple of reasons:
* We definitely see bugs in template discovery
* 3rd party packagers seem to have trouble with
building maeparser.
* Allow use in other novel places that can't use
the boost:: infrastructure required by
maeparser.
maeparser is always required for running the unit tests.
Includes a short Python script for generating the C++ files
describing the templates. This Python script currently requires
use of the Schrodinger Python tools, so it will need to be
manually updated if the templates are updated. There is a test
that will fail if a new template is added but isn't added to
the compiled templates.
run it like:
$SCHRODINGER/run mol_generator.py templates.mae
Generated using:
$SCHRODINGER/run mol_generator.py templates.mae
The version of templates.mae was:
c17d2be
Set version in CMakelists to 1.4.0. There are no API changes, but clients who build coordgen are going to need to update installation instructions because of #55 .
3 participants
See internal Schrödinger rationale here:
https://jira.schrodinger.com/browse/CRDGEN-252
Bundles templats and allow coordgen to run without maeparser.
Now templates are always bundled into the coordgen library,
so there is no search on the file system. coordgen can also
be built without maeparser support (USE_MAEPARSER=OFF). This
disables user-local template files, but it allows coordgen
to be built without reliance on the maeparser library
(and therefore without boost).
This is useful for a couple of reasons:
building maeparser.
the boost:: infrastructure required by
maeparser.
maeparser is always required for running the unit tests.
Includes a short Python script for generating the C++ files
describing the templates. This Python script currently requires
use of the Schrodinger Python tools, so it will need to be
manually updated if the templates are updated. There is a test (#53)
that will fail if a new template is added but isn't added to
the compiled templates.
run it like:
maeparser is always required for running the unit tests.
Note to reviewers
One of the commits here is entirely autogenerated code. I'd recommend avoiding looking at it because it is very long and duplicative, unless it is helpful in reading the Python file that is used to generate the code.