Fix issue #215 #216
Fix issue #215 #216
Conversation
|
It looks like the same version was installed for both the build and tests (dev not for the tests): However, when building the wrappers it states: So, it looks like Sire doesn't work with this version of RDKit. Will install locally to try to work out what the problem is. Locally I have: |
|
Okay, this is because the patch release version of RDKit appears to have removed most of the required headers, i.e.: 2024.03.4: 2024.03.5: The life of a conda package maintainer. I'll raise an issue on their feedstock. |
|
As per #217, |
|
Oh, I give up. As soon as I fix the RDKit issue it now appears that I'll check back through the logs to see how long this has been happening, but it appears that the |
|
Nope, no |
|
The version has not changed since yesterday (0.6.6) when it was detected during a previous CI run for this PR, so I'm not sure why it's not working. It certainly doesn't depend on RDKit. |
|
It seems like this fix won't work for SOMD1 since the old if (pointers[IFBOX] == 1)
{
/** Rectangular box, dimensions read from the crd file */
Vector dimensions(crd_box[0], crd_box[1], crd_box[2]);
// qDebug() << "We have a periodic box of dimensions"
// << crdBox[0] << crdBox[1] << crdBox[2];
// PeriodicBox.
if (crd_box[3] == 90.0 && crd_box[4] == 90.0 && crd_box[5] == 90.0)
{
spce = PeriodicBox(dimensions);
}
// TriclinicBox.
else
{
spce = TriclinicBox(dimensions.x(), dimensions.y(), dimensions.z(), crd_box[3] * degrees,
crd_box[4] * degrees, crd_box[5] * degrees);
}
// spce = PeriodicBox( Vector ( crdBox[0], crdBox[1], crdBox[2] ) ).asA<Space>() ;
// qDebug() << " periodic box " << spce.toString() ;
}
else if (pointers[IFBOX] == 2)
{
/** Truncated Octahedral box*/
throw SireError::incompatible_error(QObject::tr("Sire does not yet support a truncated octahedral box"),
CODELOC);
}
else
{
/** Default is a non periodic system */
spce = Cartesian();
}The second clause will need to be set to |
|
Have replaced with: if (pointers[IFBOX] == 1 or pointers[IFBOX] == 2 or pointers[IFBOX] == 3)
{
/** Rectangular box, dimensions read from the crd file */
Vector dimensions(crd_box[0], crd_box[1], crd_box[2]);
// qDebug() << "We have a periodic box of dimensions"
// << crdBox[0] << crdBox[1] << crdBox[2];
// PeriodicBox.
if (crd_box[3] == 90.0 && crd_box[4] == 90.0 && crd_box[5] == 90.0)
{
spce = PeriodicBox(dimensions);
}
// TriclinicBox.
else
{
spce = TriclinicBox(dimensions.x(), dimensions.y(), dimensions.z(), crd_box[3] * degrees,
crd_box[4] * degrees, crd_box[5] * degrees);
}
// spce = PeriodicBox( Vector ( crdBox[0], crdBox[1], crdBox[2] ) ).asA<Space>() ;
// qDebug() << " periodic box " << spce.toString() ;
}
else
{
/** Default is a non periodic system */
spce = Cartesian();
} |
|
Hmm, weirdly later in the CI output I see: So perhaps everything is actually okay? |
|
Okay, everything passing apart from Windows: @chryswoods: I recall that there was some weirdness with (I'm guessing the Boresch restraints also fail because they are using RDKit internally.) |
|
And
The versions should clearly be compatible. |
|
Looking at the previous logs (where Windows worked) it seems that RDKit wasn't found during the build, so maybe the tests passed because they were actually skipped somehow. |
|
The Boresch errors are unrelated to the RDKit changes and appear to be caused by and IO error loading the fixtures for the tests. Hopefully those will be fixed by a simple re-run. |
|
Yes, the latest run solved the Boresch issues. Now we just have the RDKit issues on Windows: As mentioned above, I'm not sure we had actually be previously compiling the RDKit support on Windows, so this could be a longstanding issue that we've only just discovered. Not sure how to debug this without access to a Windows box or VM. |
|
RDKit support did work on Windows, so it is a regression to lose it. I will look to debug next week. |
|
Thanks, I thought that was the case but recall it being a pain to get it working. |
This PR closes #215 by setting the
IFBOXpointer to 3 for triclinic spaces. The value of 2 is reserved for a truncated octahedron. I'm not sure if we should add logic to detect this, or whether the use of 3 should suffice for everything. I'll test and report back if there are any issues.Note that I've killed this superses #214, so I've killed the CI for that PR.
This also closes #217 by switching to the
librdkit-devpackage as a replacement for the now deprecatedrdkit-dev.develinto this branch before issuing this pull request (e.g. by runninggit pull origin devel): [y]Suggested reviewers:
@chryswoods