Merge pull request #5016 from RosettaCommons/vmullig/fix_iterator_copy_issue
Fixing clang 12 build for MacOS.
Clang 12 warns about unnecessary copying when using auto as an iterator in a for loop.
notify author
notify list [rosetta-logs@googlegroups.com]
Merge pull request #4945 from RosettaCommons/jkleman/benchmarks
Jkleman/benchmarks:
fixing a bunch of scientific tests but there is more work to do. Just want master to be up to date so I can see what else needs fixing.
notify author
notify list [rosetta-logs@googlegroups.com]
Merge pull request #5005 from RosettaCommons/vmullig/fix_modernize_use_nullptr_warnings
Trying to fix clang-tidy warnings
They're currently making a lot of red on the test server. These warnings are false alarms, and can be safely ignored, but I don't like having a lot of red lights that people are trained to ignore.
So here there's a bit of a problem. The original point of pull request #4941 (which created these false alarms) was to deal with the fact that if a developer wrote `tag->getOption<bool>( "myoption", "false" )`, the string literal would be interpreted as a boolean `true`. To get around this, it was necessary to introduce a `Tag::getOption( std::string const &, char * const )` variant, but that means that `tag->getOption<core::Size>( "myoption", 0 )` gets interpreted as `nullptr` (even if I explicitly delete the variant for core::Size). This means I have to explicitly delete the variant for core::Size _and_ explicitly say `tag->getOption<core::Size>( "myoption", core::Size(0) )`, which is a bit frustrating...
<ins>Edit:</ins> Rocco suggested implementing `Tag::getOption( std::string const&, int const)`, since `int` is a "preferred" interpretation of `0` (over `core::Size` or `nullptr`). This seems to work, and ensures that `tag->getOption<core::Size>( "myoption", 0 )` can still be used as it was previously. I'll leave the places where I converted to `tag->getOption<core::Size>( "myoption", core::Size(0) )` since they're harmless, but I won't bother converting the rest. Note that I am deliberately deleting the Real and bool versions of the int `getOption`, since doing so has revealed places where people were setting the wrong type. (So initializing a Real requires `tag->getOption("myoption", 0.0)`, and `tag->getOption("myoption", 0)` will result in an error. If people really hate this, we can switch it back later, but it does catch bugs.)
<ins>Note to reviewers:</ins> I know that a lot of files have changed here, but most changes are trivial. There are five types of changes in this PR:
1. Changes to Tag.cc/Tag.hh. <b>These are the most important.</b>
2. A few places where the changes revealed the wrong type of datum being set (e.g. setting a bool with a tag parsed as a core::Real). The `SidechainMover` is one example. <b>These are not so important.</b>
3. Lots of places where `tag->getOption<core::Real>("myoption", <some int>)` was replaced with `tag->getOption<core::Real>("myoption", <the equivalent float>)`. These are largely cosmetic, but they allow us to be more pedantic about the structure of `getOption` so that we can catch the errors in number 2. <b>These are not so important.</b>
4. Some places where I was changing `tag->getOption<core::Size>("myoption", 0)` to `tag->getOption<core::Size>("myoption", core::Size(0))` before Rocco suggested a fix that no longer necessitated this. I've left these since it was too much trouble to change them all back and since they do no harm. <b>These are not so important.</b>
5. Addition of the `OLDER_CLANG` definition for clang < 4.0. This allows me to get around the problem described below.
<ins>Added note:</ins> this PR revealed a bug in older versions of clang. In clang 3.4 (on the test server), it seems you can't have a deleted general template followed by an explicitly-defined template specialization (which is useful to say, "I don't want this function to accept anything but the types that I define, and I want a compiler error otherwise"). I'm special-casing that here by defining OLDER_CLANG when compiling with clang < 4.0, and implementing a general version of `T Tag::getOption<T>(std::string const &, int const) const` that throws an error if invoked if OLDER_CLANG is defined (using `delete` to turn this runtime error into a compile-time error for newer compilers). So for older clang we have the following pseudo-code:
```c++
class Tag {
...
template< class T > T getOption( std::string const &, int const ) const { /*Throw error here at runtime if ever called.*/ }
...
};
template <> core::Size Tag::getOption( std::string const &s, int const i ) const { /*Actually parse the tag without error.*/ }
```
On newer clang or on gcc or icc, it becomes:
```c++
class Tag {
...
template< class T > T getOption( std::string const &, int const ) const = delete; //Compiler error instead of runtime error -- better for catching stuff (e.g. a `bool` or `string` initialized with `8`).
...
};
template <> core::Size Tag::getOption( std::string const &s, int const i ) const { /*Actually parse the tag without error.*/ }
```
notify author
notify list [rosetta-logs@googlegroups.com]
Build `zlib` from sources (#4968)
Build `zlib` from sources
Issue Details:
Rosetta historically has relied on `zlib` existing on the OS, but Windows doesn't ship with `zlib`.
Fix Details:
Use the `zlib` sources checked in to Rosetta to build `zlib` for Windows only.
notify author
notify list [rosetta-logs@googlegroups.com]
Merge pull request #4997 from RosettaCommons/vmullig/non_recursive_refold
Attempt to remove recursion in internal coordinate updates
Our current internal coordinate update logic can result in call stacks thousands of layers deep. When stack size is limited, this can result in stack overflows. Andy and I have been running into this working with larger poses in threads on MacOS, where the stack size of non-primary threads is limited to 512 kB (something that it's not possible to override when using std::threads).
Switching to non-recursive logic will probably have benefits for performance even outside of a multi-threaded context, so let's give this a try.
This is complicated by the fact that, given a parent atom P and children C1...CN, the stub used to update C1 is modified by the updating operation _and then_ passed to C2 to update it (where it changes again). So the existing recursive logic was much easier than a non-recursive iterative approach, since it's not straightforward to store the data that are needed to compute each child. The new logic is to iterate over atoms at a given level of depth, updating those atoms' coordinates while constructing two lists: a list of unique stubs with which to update their children, and an ordered list of pairs of (child atom, pointer to stub to use to update). In the next iteration of batches of parents, the old children become the new parents and the old children's stublist becomes the new parents' stublist, parents' coordinates are updated in sequence (suitably modifying the stubs in the parent stublist), and a new children's stublist and children list is populated. This continues until a generation produces no children. This is summarized below:
Old:
- Start with root atom and its stub.
- Recursive function:
- Update current atom with stub (updating stub in the process).
- Construct child's starting stub.
- Loop over all children:
- Recursively call this function, passing in a child and the child's stub (both of which will be modified).
New:
- Allocate space for list of parents' stubs, ordered list of parents.
- Allocate space for list of children's stubs, ordered list of children.
- Put root atom and its stub into ordered list of parents and list of parents' stubs, respectively.
- Do:
- Loop over current parents:
- For each parent, update current atom with stub (updating stub, owned by parent stublist, in the process).
- Construct child's starting stub, and store it in the children's stub list.
- For each child, store pair of (child, pointer to child's stub in children's stub list) in children's list.
- Delete parents' stub list and list of current parents.
- Children's stub list and children's list become parents' stub list and parents list, respectively. (Done with dual buffers and pointer swap for efficiency).
- Break if new parents list is empty (i.e. no children from last round).
A few details:
- I'm avoiding owning pointers due to the cost of incrementing and decrementing reference counts.
- I am using raw pointers, but only for accessing objects that are owned by something else (where the lifetime is longer than the function in which I use raw pointers), which is no more dangerous than using references, or for objects owned by std::deques where the lifetime is longer than the raw pointers (again, no more dangerous than using references).
- Since appending to a deque doesn't invalidate references to existing elements, storing pointers to those elements is safe.
Tasks:
- [x] Switch internal logic to a non-recursive algorithm based on ordinary iteration.
- [x] Debug.
- [x] Check unit tests.
- [x] Check integration tests.
- [x] Check performance tests.
- [x] Check profile tests.
- [x] Check scientific tests --> simple_cycpep_predict unchanged.
- [x] Beauty.
notify author
notify list [rosetta-logs@googlegroups.com]
Improve LocalRelax to work with DNA and be non-directional (#4971)
I had a complex I wanted to use LocalRelax on but because of the directionality code it was incompatible with DNA.
These changes remove the directionality, but allow non-protein residues to work with this code!
I also updated a few XRW_TODO comments, and compared this to the previous implementation.
compared to the previous LocalRelax, this code packs slightly more residues at a time (from ~140 -> ~200) but keeps approximately the same number of residues minimzing at each step (~50).
K used to be some function of the distance between two CA/CB residues * directionality parameters * 0.28. now it's just based on the NBR_radius so it's triggered when the distance between two nbr atoms is <= nbr_radius_1 + nbr_radius_2.
That's what I found to make the # residues that minimize/pack the most similar to the old version
notify author
notify list [rosetta-logs@googlegroups.com]
Merge pull request #5000 from RosettaCommons/revert-4999-revert-4714-roccomoretti/rdkit_submodule2
Revert "Revert "Add RDKit as a submodule to Rosetta.""
notify author
notify list [rosetta-logs@googlegroups.com]