-
Notifications
You must be signed in to change notification settings - Fork 131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adaptive search #192
Adaptive search #192
Conversation
@@ -474,6 +476,12 @@ def link_df(features, search_range, memory=0, | |||
the next frame. | |||
|
|||
For examples of how this works, see the "predict" module. | |||
adaptive_step : float (optional) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is picky, but I have been parsing numpydoc formatted strings recently and the convention is
name : type, optional
Left some trivial-ish comments from reading the code. My knee-jerk reaction is to be skeptical, but I am having trouble coming up with a good reason why this is worse than hand tuning. |
Thanks, @tacaswell. I've just force-pushed a rebased branch that addresses your line comments. I'm glad you think that the example notebook gives users a sense of when adaptive search is and is not a good idea. But I'm not quite satisfied with that yet because there's presently no diagnostic info related to subnets or adaptive search — we're warning users that they can shoot themselves in the foot, but it's as though they can't even see where the darn thing is pointed. So as I mention in my comment to #184, some debug-level logging would go a long way here, and should be added before we merge. |
Inserting @tacaswell 's #184 (comment) into this discussion:
|
+1 for |
fd3580c
to
1d0f6f3
Compare
The way that such a condition could arise is no longer known.
This requires separating out the tests that require a normal subnet linker.
This also fixed a bug involving chained indexing
1d0f6f3
to
d65f516
Compare
This was causing the linking tests to fail occasionally.
Lots of new stuff in this rebase/push. Highlights:
Obviously we have greatly expanded the scope of the original PR. But the diagnostics were motivated by adaptive search, and they mess with some of the same pieces of code, so I decided to take the lazy approach and then get feedback. |
neighbor_strategy=neighbor_strategy, link_strategy=link_strategy, | ||
hash_size=hash_size, box_size=box_size) | ||
|
||
if diagnostics: | ||
features = strip_diagnostics(features) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment noting that strip_diagnostics
does not modify the original? I was worried for a moment that it did.
Thanks @danielballan . Good catches. I'll fix those issues, and let me know whether you want the new stuff in its own PR. Also on my to-do list before a (possible) merge:
|
@@ -58,6 +81,13 @@ def test_one_trivial_stepper(self): | |||
assert_frame_equal(actual, expected) | |||
actual_iter = self.link_df_iter(f, 5, hash_size=(10, 2)) | |||
assert_frame_equal(actual_iter, expected) | |||
if self.do_diagnostics: | |||
assert 'diag_search_range' in self.diag.columns | |||
print(self.diag.diag_search_range) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops! Will remove this print
and check for others.
Sounds good to me. Good idea with |
Also added doc/tutorials/index.rst to .gitignore; it is automatically generated by the Makefile.
The promised changes are now up. Also, I wrote a new tutorial on linking diagnostics, and added diagnostics to the tutorial on adaptive search. See soft-matter/trackpy-examples#17 . The 2 tutorials in question are: |
The linking diagnostics tutorial is just so very cool. Wow. I think it's time to merge this and the examples PR. Any final revisions? |
I gave these a read over a while ago and had nothing to say but 'cool!' I can give a more thorough code review if you want, but time scale for that On Thu, Feb 12, 2015, 08:32 Dan Allan notifications@github.com wrote:
|
OK, I'm going for it. I know first-hand how busy you are. :- D |
DOC : Update API docs after #192.
This PR implements a new feature I call "adaptive search," that is explained and demonstrated in two new tutorials. See soft-matter/trackpy-examples#17
Adaptive search is my attempt to address an age-old problem when tracking dense packings with Crocker-Grier: how to select a
search_range
that does not exclude valid links, without creating many large subnets that make linking impractical or impossible. Conventionally, this is done by guessing, followed by trial, error, frustration, and resignation. However, when I was tracking 30k particles over 12k frames, it made no sense to carefully choose a single value for all particles in all frames — asearch_range
that worked perfectly well for the first 5500 frames or so would cause aSubnetOversizeException
in frame 5501, because a corner of the image was momentarily displaced.With adaptive search enabled, one instead specifies a maximum
search_range
for the entire movie. If an oversize subnet is encountered, linking becomes re-entrant: the particles of the offending subnet are re-linked with progressively smaller values ofsearch_range
. In this way, solvable (or trivial) subnets are broken off and solved one by one, until there are no more particles left to link. When it works, this is a huge simplification for the user: guess a reasonable maximumsearch_range
based on the largest particle displacements, and leave the rest totrackpy
.Implementing this feature required a few big internal changes to the linker. The ones worth mentioning are
MAX_SUB_NET_SIZE
and its associated logic, and providing an alternate value more suited to the adaptive search algorithm.This is a big one — it's taken roughly the past year to dream this up, tell @danielballan about it, implement it, use it in my research, rebase, rebase, rebase, and write the documentation. (Although most of the heavy lifting was done by @danielballan and @tacaswell when they wrote the linking tests.) Some serious scrutiny is needed, though if there are no major problems it would be good to target this for the v0.3 milestone. Thanks!