ANMD and ClustENM fixes and test fixes - #2269
Open
jamesmkrieger wants to merge 13 commits into
Open
Conversation
Adds a checker for the blocks used by the rotating translating blocks models, so that they are validated in one place rather than partly and late. It checks that the blocks are list like, that their identifiers are all of one type, that they are one dimensional, that there is one per node, and that they leave more degrees of freedom than a rigid body has. The identifiers are counted rather than used as indices, so they can be of any hashable type, such as strings. Mixed types are rejected because counting them by value and coercing them into an array disagree: 0 and '0' are two blocks by value but one after coercion. The degrees of freedom are counted the way the block Hessian itself is built, where a block holding a single node contributes 3 rather than 6, having nothing to rotate. Two blocks of one node each therefore leave the same 6 degrees of freedom as a single block of everything, so counting the blocks alone would not catch it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
RTB checked that the number of blocks matched the number of atoms but
not that they left anything to sample, so blocks describing a single
rigid body were projected and only failed afterwards in calcModes, with
an eigenvalue index range that says nothing about the blocks:
ValueError: Requested eigenvalue indices are not valid. Valid range
is [0, 5] and start <= end, but start=0, end=7 is given
Both checks now come from checkBlocks, which imANM inherits, so the
blocks are rejected where they are given. Adds the first tests of imANM,
covering blocks of the wrong length, blocks that leave a rigid body,
identifiers that are not numbers, and identifiers of mixed types.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
writePDBFixed now takes the filename to write, and returns the one it
wrote, so that a caller no longer has to reconstruct the name from the
title. runANMD needs this: it replaces the spaces of the title with
underscores while writePDBFixed did not, so a title containing a space
made the two disagree and the run failed with FileNotFoundError on a
file that had just been written under another name. The file it writes
is also closed now rather than left to the garbage collector.
Three input checks in run():
- the threshold size mismatch formatted %d against the tuple of
thresholds instead of its length, so it raised TypeError from inside
its own error message rather than the ValueError it meant to. The
maxclust branch above it already used len().
- running without having set atoms gave TypeError: 'NoneType' object
is not subscriptable from indexing them for the Kirchhoff matrix.
- the blocks of the block based subclasses were only checked once
their ANM was built, after a structure had been fixed and a
generation minimised. They are now checked against the coarse
grained nodes they have to describe before any of that, and the
count they are checked against is named in the message, being the
nodes rather than every atom of the fixed structure.
Adds tests for ClustENM, covering run() arguments, the state of a new
instance, titles, the block based subclasses, writePDBFixed, fixing a
structure, and a small run. The subclass checks for blocks are made
again with blocks set, where the parent's checks would otherwise be
masked. Zero generations is covered as the supported way to minimise a
structure without sampling, which is why the clustering parameters are
not needed in that case.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
runANMD imported OpenMM only through the legacy simtk namespace, which
is a deprecated shim, and did so before validating its arguments. So
without OpenMM installed the five argument type tests failed with
ImportError: Please install PDBFixer and OpenMM to use ANMD
instead of the TypeError they assert, because rejecting a bad argument
had been made to depend on a simulation package it does not need.
The arguments are now validated first, and the import tries the modern
openmm namespace before falling back to simtk, as addMissingAtoms
already did. ANMD was the last place importing only the legacy one.
This also drops the deprecation warning the shim emits.
OpenMM and PDBFixer are optional, so the tests that run ANMD are now
skipped rather than failed when they are missing, with a warning so the
gap is not silent. The argument tests need neither and so always run.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The interaction tests removed three of the four .npy files they save, leaving test_3o21_disu.npy in the working directory after every run, and removed them unconditionally, so a test that failed before saving one raised FileNotFoundError from tearDownClass and reported that in place of the failure which caused it. The buildMSA tests removed nothing at all. buildMSA writes the sequences it aligns beside its output, both named after the title, and clustalw adds an .aln alignment and a .dnd guide tree of its own, so a run left Unknown.fasta, Unknown.aln and Unknown.dnd behind. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
buildMSA has aligned with clustalo as well as clustalw for some time, but neither its docstring nor its tests said so, and the same went for the outfilename argument. Documents both and adds clustalo to environment.yml beside clustalw, which bioconda serves for every platform the CI runs on. The new tests cover aligning with clustalo, naming the output after a given title, and writing it to a given outfilename. clustalo gives an alignment of its own rather than the one clustalw gives, 374 columns against 399 for these sequences, so it is checked for being a valid alignment of the sequences given, by its labels, its column count and the sequences it holds once the gaps are taken out, rather than against a stored alignment that would also pin the version. Neither program is a required dependency, so the tests that need them are now skipped rather than failed when they are missing, as the dssp tests already do, with a warning so that the gap is not silent. The Biopython test needs neither and so still always runs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
_prep_sim asked for {'Precision': 'single'} when no platform was given, so
it passed platform-specific properties without a platform to go with them.
OpenMM discarded them silently up to 8.0, but 8.1 onwards raises
ValueError: Cannot specify platform-specific properties, because the
Platform is not specified
which failed every ClustENM run that did not name a platform, and so is
seen with an installed OpenMM but not an older one. Nothing to do with the
absence of a GPU: the properties asked for single precision, which is what
CUDA and OpenCL are given, but they were asked for whatever platform OpenMM
would have picked.
Properties are now only given for a platform we chose, leaving OpenMM to
pick the fastest it has with its own defaults otherwise, which is what it
already did in effect. The CUDA, OpenCL and CPU cases are unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Covers running without naming a platform, which is the case that regressed, and on the CPU, CUDA and OpenCL platforms, each of which ClustENM gives different properties. Which platforms OpenMM has depends on how it was built and on the hardware it finds, so the tests of the ones it does not have are skipped rather than failed, with a warning for the accelerated ones so that not having tried them is not silent. Reverting the previous commit fails testNoPlatform and leaves the rest passing, so these hold that fix rather than only running beside it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A generation samples n_confs conformers and then filters and clusters them,
so how many it keeps depends on the conformers themselves, and so on the
modes the eigensolver of the machine gives. The block based runs keep one of
the two here and two on the CI, which failed
ClustRTB run with two blocks failed to give the conformers of both
generations: ACTUAL: 3 DESIRED: 2
so the count is not a property of the run to be expected in advance. The
tests now read it and check what does hold: generation 0 is the minimised
starting structure alone, generation 1 keeps at least one conformer and no
more than it sampled, and the totals, keys, labels, coordinate sets and
potential energies all agree with it. Deriving them keeps the relations
between them just as strictly, and still fails if a generation samples
nothing or keeps more than it sampled, while not pinning a number that
belongs to the machine.
Also takes n_confs and maxclust for the block runs from the same attribute
the bound is read from, so the two cannot drift apart.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The macOS runners offer the OpenCL platform without a device it can use, so
testOpenCLPlatform was not skipped and failed in the run itself:
openmm.OpenMMException: No compatible OpenCL platform is available
Being one of the platforms OpenMM lists only says the plugin loaded, so each
is now tried, by making a context of a single particle on it, rather than
looked up in the list. The warning and the skip reasons say that OpenMM
cannot run on it here rather than that it does not have it, since the two
are different and only the second is about how it was built.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
setAtoms now takes a list of structures, which become the generation 0 population rather than the single minimised starting structure one structure gives, so generation 0 holds as many conformers as it was given and the keys and labels run 0_0, 0_1, ... before generation 1 begins. Nothing covered that. Covers the initial population, its keys and labels, the generation sampled from it, starting from three structures rather than two, a list of one behaving as a single structure does, an empty list, and structures that are not the same molecule, whose coordinates cannot fit the topology built from the first. How many conformers the sampled generation keeps is counted rather than expected, as the single start tests do. The models of a local NMR structure stand in for the conformers a run would start from, so nothing is fetched. The tests that only read a run from every model share one, as the single start run tests do. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
@karolamik13 all tests pass including ones for clustenm multi-start |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Also includes improved checker for RTB blocks
This should fix the ANMD test problems mentioned in #2264 - @kontheodosiadis please confirm
This is also a prerequisite that more clustenm tests for #2258 will be built upon