-
Notifications
You must be signed in to change notification settings - Fork 132
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
FIX Gaps in frames are not ignored anymore #293
Conversation
Is it possible to change the behavior of |
link_df_iter is unchanged compared to master
On second thought, I agree, the generators So |
The code changes look good. Would it be OK if I added a warning when a skipped frame is encountered? Or just a debug message? My worry is that if the user ever has floating-point frame numbers, and they are integer + epsilon, the resulting problem will be a nightmare to debug. |
@nkeim What is the use-case for floating-point frame numbers? |
@tacaswell Besides making mischief? The main use case is if you've read the data directly from format that makes it hard to mix types within an array (like vanilla HDF5). |
By adding 0.5 to the expected framenumber, this can be fixed. Speaking about column types: the particle label type is now float (because |
@caspervdw Fabulous! Much better than a warning. I agree that integer particle IDs are desirable. Maybe we can do int64 and initialize with -1. Will create an issue. |
Fixed. Tests pass locally. Travis keeps failing btw because of pims. We should make the PIL/Pillow and scipy dependencies compeletely optional (see soft-matter/pims#186) |
As of the latest pims fix, the tests pass again. Ready for merge. |
@@ -1009,8 +1049,9 @@ def link(self, levels): | |||
p.forward_cands = [] | |||
|
|||
# Sort out what can go to what. | |||
assign_candidates(cur_level, prev_hash, self.search_range, | |||
self.neighbor_strategy) | |||
if len(cur_level) > 0 and len(prev_hash) > 0: |
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.
Does assign_candidates
not deal with empty levels gracefully on it's own?
Great! Merge for v0.3, or v0.3.1? This seems like a non-critical bug for the vast majority of existing users, and the fix could break existing code. So I think that we should either issue v0.3.0rc2, or hold until v0.3.1. I vote for the latter. |
Same. On Mon, Oct 26, 2015 at 2:50 PM Nathan Keim [email protected]
|
Agreed |
Hi! I did this 1 week ago, but I still have the problem, it seems like the amount of gaps is lower, but I still have some. Looking foward for any comments :) |
That must be a separate issue then.. could you post the portion of the dataframe that has this unexpected gap? |
Sure, here it is: frame x y mass size ecc signal raw mass ep frame You can find two gaps, one in the frame 5135-5137 and the second in the frame 5148-5150. Im adding also an image for you to see the information in a more orderly way: |
So is the issue then that these features are missing? Or that the features are linked into one trajectory while they actually should not (with memory=0). I don't see a |
That´s right, the issue was that the features were linked into one trajectory while they should not because of memory=0. |
That's good. @nkeim, I think this is ready to merge. Also we shoukd consider doing a minor bugfix release soon. |
Sorry, I don't know how I missed that this was ready. I should be able to look at it tomorrow. Is there anything else for 0.3.1? I see that #286 is also tagged. |
@caspervdw Apologies again for the long delay. I think @tacaswell has a good point. The BTree hash has no problem with empty levels; it is just an idiosyncrasy of KDTree, so I think it is better to have the workaround at the lowest layer of abstraction. I have created caspervdw#2 in case you would prefer to do it that way. But I could be very easily convinced to merge as-is 😄. |
Never mind — my PR does not have all the recent Travis fixes. Will instead merge and create separate PR against master. |
Hi Casper, I´m the one that posted about the problem with the gaps in trackpy and then 2016-04-19 16:11 GMT+02:00 Casper van der Wel [email protected]:
2872 184,7847837 133,1168551 14378,45113 4,171569113 0,211527342 264,2620271 92361 2872 233 |
That's strange, the tests confirm the correct working of the code. Is it possible that you started using trackpy v0.3 again and not the dev version (check If this is all OK, could you do some debugging to narrow down the issue? Or give me a recipe on how to reproduce the problem? Please make a new issue if the problem persists, as this PR is closed. |
Hi Casper, Cecilia Rodriguez 2016-05-09 18:51 GMT+02:00 Casper van der Wel [email protected]:
|
Fixes #292 , but not yet for
link_df_iter
. There is an exception because I only changed thelevels
generator and inlink_df_iter
, this is zipped together with the original iterable, which is unchaged.Do you agree on the approach?
Another approach would be
link_iter
checking on the framenumbers.