Project

General

Profile

Task #866

Improve analysis neighborhood search API

Added by Teemu Murtola about 8 years ago. Updated almost 6 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
analysis tools
Target version:
Difficulty:
uncategorized
Close

Description

NeighborhoodSearch class in source:src/gromacs/trajectoryanalysis/nbsearch.h is a very thin wrapper around original C code in source:src/gromacs/selection/nbsearch.h. It would be best to just convert the C code into C++, improving the interface in the process, and get rid of the wrapper. Would be good to add unit tests at the same time to make sure that the converted code works.


Related issues

Related to GROMACS - Task #827: Improve interfaces in analysisdata, selection and trajectoryanalysis modulesClosed12/22/2011
Related to GROMACS - Task #651: Increase coverage of selection unit testsClosed01/09/2011

Associated revisions

Revision 701fb22b (diff)
Added by Teemu Murtola almost 8 years ago

Fix more Doxygen warnings.

- Add documentation for some partially documented classes.
- Some fixes to cosmetic issues in surrounding code that were noticed.
- Some other clean-up.

There are a few warnings in pre-5.0 C code that are not addressed here.
Of the remaining warnings, quite a few are are either in obsolete code
(template.c, displacement.c), or in code that should be heavily
refactored (nbsearch.h, issue #866).

Change-Id: I7c309153769f35e93c4d0ff28667663e6a9e8ee3

Revision e578a58c (diff)
Added by Teemu Murtola almost 8 years ago

Merge two separate nbsearch.h files.

Very small part of IssueID #866

Change-Id: I064ee13f31f199c8c7d455ab04445046028d5727

Revision dabd9a3c (diff)
Added by Teemu Murtola over 6 years ago

Proper C++ API for analysis neighborhood search.

Replace the dummy C++ wrapper with a more C++-like API for the
neighborhood search routines that are part of the selection code.
Subsequent commits will implement a few additional features to the API,
but split those off from here to keep things easier to review.

Implementation underneath is still the same as earlier.
Will remove the old C functions in a separate commit.
Changed the free volume tool to use the C++ API in preparation for this.

Added some basic tests for the functionality using the new API, which
should increase the percentual coverage in this part of the code
significantly.

Part of #866 and #651.

Change-Id: I139866b82cb048f1676d187b0cf6e5d270d4d32b

Revision 49ec30f1 (diff)
Added by Teemu Murtola over 6 years ago

Fix analysis neighborhood search for 2D/screw PBC.

The gridding implementation did not work correctly in these cases, but
could get triggered. For now, a simple all-pairs search is always used
in these cases.

Related to #866 and #651.

Change-Id: Iad066f3ea5964e97e1a29a0a20c5725f205509c8

Revision 1adde8a1 (diff)
Added by Teemu Murtola over 6 years ago

Better concurrency support for analysis nbsearch.

AnalysisNeighborhood::initSearch() now supports concurrent calls from
multiple threads such that each call creates an independent search.
A pool of search objects is kept allocated to avoid constant
reallocation of the data structures.

This allows using the neighborhood search routines transparently in
thread-parallel analysis tools after that is implemented in the
framework. The changes in the template demonstrate how this simplifies
the tool code.

Part of #866.

Change-Id: If469284671ec66e249e138e59da7cc87a8f4ac65

Revision a4a0693a (diff)
Added by Teemu Murtola over 6 years ago

Remove C API for analysis neighborhoor search.

Makes it easier to refactor the C++ API, when no consideration needs to
be put into maintaining the C interface. If really necessary, this can
be reimplemented on top of the C++ API later.

Nearly the whole nbsearch.cpp file changes, but most of this is
mechanical replacement. Kept the order in the code as same as possible
to make it easier to see that the logic is not changing.

The high-level changes are:
- Move members of gmx_ana_nbsearch_t to the appropriate implementation
classes and suffix them with an underscore.
- Move static helper functions into members of the appropriate
implementation classes. This and the above are responsible for
majority of the changes, as 'd->member' becomes 'member_' on most
lines.
- Move the declarations of the C++ implementation classes to the
beginning of the file.
- Move all Doxygen documentation to the classes.
- Restructure the C++ classes a bit to remove some workarounds that
were necessary to use the C API underneath. Most notably, the forced
search mode is now passed directly to the initialization function,
removing some extra boolean flags, and the const methods in
AnalysisNeighborhoodPairSearch are now really const.
Also some minor refactoring included here:
- Remove use of cbrt(), and thus dependency on config.h.
- Some conversion to C++ patterns: use of static_cast and moving
variable declarations close to the first use.
Will continue the clean-up in subsequent commits to make it easier to
review that this part really does not change the old functionality.

Part of #866.

Change-Id: I40be305704e17d58f8cb906ad5fa69ea179ece15

Revision 902c99cb (diff)
Added by Teemu Murtola over 6 years ago

C++-ify analysis nbsearch more.

- Use static_cast instead of C casts throughout.
- Use more C++-style comments.
- Use the same pattern for startPairSearch() as for initSearch() to
allow multiple concurrent searches.
- Use std::vector instead of explicit memory allocation for the grid
cell management. Memory is still managed explicitly for rvec and ivec
arrays, since they can't be put into std::vector.
- Make most members of the implementation classes private to clarify the
code (the implementation class is now explicitly responsible for
managing its internal state).

Part of #866.

Change-Id: I2af5552b696644f954957f96705baabd891284a9

Revision 4b19212b (diff)
Added by Teemu Murtola over 6 years ago

More flexible position input for nbsearch routines.

In addition to single positions, it is now possible to do a full
pairwise search between two set of positions. Introduced a helper class
that encapsulates all information about the positions, such that all
methods can take this class as input, and all different forms of
initializing the positions only needs to be implemented once in this
class. Also made it possible to pass selections directly to the
neighborhood search routines and removed deprecated
Selection::positions(), which was only useful in combination with the
nbsearch code.

Part of #866.

Change-Id: I1829d981ddff16a98c5a2bae873cc3589ef44d4a

History

#1 Updated by Teemu Murtola about 7 years ago

  • Target version set to future

Not the most pressing issue for 5.0, although would be easier to do before there are a lot of tools that use these interfaces.

#2 Updated by Teemu Murtola almost 7 years ago

  • Status changed from New to In Progress
  • Assignee set to Teemu Murtola
  • Target version changed from future to 5.0

Decided to implement this to the end in an effort to reduce the number of stuff I have in progress. The main motivation is to get better test coverage for this part of the code, and those tests are easier to implement against a proper C++ interface (and the interface needs some additions to make it easy to test the different paths). But as said earlier, a proper C++ interface is also nicer to have for writing new code.

#3 Updated by Teemu Murtola over 6 years ago

  • Status changed from In Progress to Fix uploaded

Changes leading up to https://gerrit.gromacs.org/#/c/2382/ implement this to a reasonable level.

#4 Updated by Teemu Murtola over 6 years ago

  • Status changed from Fix uploaded to Resolved

All patches merged.

#5 Updated by Teemu Murtola over 6 years ago

  • Status changed from Resolved to Closed

#6 Updated by Teemu Murtola almost 6 years ago

  • Project changed from Next-generation analysis tools to GROMACS
  • Category set to analysis tools

Also available in: Atom PDF