-
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
annotate with two different colors based on threshold #104
Conversation
@@ -128,8 +128,8 @@ def plot_traj(traj, colorby='particle', mpp=1, label=False, superimpose=None, | |||
ptraj = plot_traj # convenience alias | |||
|
|||
@make_axes | |||
def annotate(centroids, image, circle_size=170, color='g', | |||
invert=False, ax=None): | |||
def annotate(centroids, image, circle_size=170, color='g', color2='purple', |
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.
You should add all new (kw)arguments at the end to maintain back-compatibility. You can call this function with nothing but positional arguments, change change will break the code for anyone who passes 5 or more positional args.
Thanks for the PR! This seems useful. What do you think about this usage:
which would also allow usages like
where The main point of my suggestion is passing colors as a list, which is in my opinion more idiomatic and natural than Let me know what you think. Thanks again. |
I agree that the list implementation is a nice way to do it. Would that be backwards compatible though? Maybe we'd have to take special care to handle it if the color passed was a single string rather than a list. What should the description for the color be that is more generic than "string". I'd be happy to change it to whatever concisely captures all of the ways to format a color. I think "string" is what was there before for color when there was just a single color. I just copied that. |
I think something like:
Would do this for an arbitrary number of splits and be back-compatible. You get some strange naming ( |
Thanks for the feedback. I updated the code in the pull request to allow for splitting into an arbitrary number of sections, got rid of the np.where()'s, and made it backwards compatible. I tested it with these examples:
|
|
||
if color is None: | ||
color = 'g' | ||
if not isinstance(split_thresh, list): |
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.
should be split_thresh is not None and ...
I think
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 to handle the case where someone puts a single number in as a threshold rather than a list. It must be converted to a list because later split_thresh[0] needs to return a value. Possible split_thresh: [10], 10, 10.0, [10,12,13], etc.
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.
Some people (like me, sometimes) have the habit of using tuples for sequence literals. isinstance(split_thresh, (list, tuple))
would save those folks from seeing a confusing exception.
I know I haven't said much on this PR, but I'm looking forward to this being in trackpy.
Conflicts: trackpy/plots.py removed white spaces Conflicts: trackpy/plots.py finished implementing the ability to section particles into different colors in annotate plot cleanups
annotate with two different colors based on threshold
|
||
if color is None: | ||
color = 'g' | ||
if not (split_thresh, Iterable): |
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.
I know the train's already left the station, but I don't see how this test wouldn't always evaluate to True. Did you mean to write if not isinstance(split_thresh, Iterable)
?
In any case, a string also counts as iterable, so there has to be another way.
Sorry I've been doing nothing but writing comments. I'll be back on my development computer by tomorrow, I promise :).
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.
My fault. This is what I get for late-night merging.
On Wednesday, July 9, 2014, Nathan Keim notifications@github.com wrote:
In trackpy/plots.py:
Returns ------ axes """ import matplotlib.pyplot as plt
- from itertools import tee, izip
- from collections import Iterable
https://docs.python.org/2/library/itertools.html
- def pairwise(iterable):
"s -> (s0,s1), (s1,s2), (s2, s3), ..."
a, b = tee(iterable)
next(b, None)
return izip(a, b)
- if color is None:
color = 'g'
- if not (split_thresh, Iterable):
I know the train's already left the station, but I don't see how this test
wouldn't always evaluate to True. Did you mean to write if not
isinstance(split_thresh, Iterable)?In any case, a string also counts as iterable, so there has to be another
way.Sorry I've been doing nothing but writing comments. I'll be back on my
development computer by tomorrow, I promise :).—
Reply to this email directly or view it on GitHub
https://github.com/soft-matter/trackpy/pull/104/files#r14724711.
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 bit of code is to handle the case where split_thresh is a single number. I turn it into a list because split_thresh[0] needs to be valid
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.
I think that the code, as written, will never put anything into a list, scalar or not. A multi-element tuple always evaluates to True.
I can say that I definitely get the wrong behavior when I try this code at the IPython prompt.
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 the case I was trying to handle:
tp.annotate(f, frames[0],color=['purple','green'],split_category='signal',split_thresh=10)
I see now that it is broken. What is the proper way to check if something is iterable?
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.
isinstance(foo, Iterable)
will return True
if foo is iterable (and properly implements the ABC stuff from collections).
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.
Thanks, Tom! I've been doing it a much less elegant way. I guess the only complication is if you'd like strings to count as single values; then you'd need something like isinstance(foo, Iterable) and not isinstance(foo, str)
. But it looks like that's not the case in this code.
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.
A wonderful phrase I learned last night, 'duck typing brings a can of worms: either you go fishing with them or eat them'
I made this change in my local branch. How should I go about submitting this one little fix to the main repository? A new pull request? |
A new PR is best, it is only easy to add commits to a PR before it has been merged. |
I added the capability to annotate an image with features of two different colors. I found this useful for playing around with cutoff thresholds. e.g. see how many features you miss if you increase the minimum size.