Skip to content
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

Fst gives a NoneType error when no indices are given (the default) #250

Closed
gtsambos opened this issue Jun 29, 2019 · 9 comments
Closed

Fst gives a NoneType error when no indices are given (the default) #250

gtsambos opened this issue Jun 29, 2019 · 9 comments
Labels
bug Something isn't working
Milestone

Comments

@gtsambos
Copy link
Member

I have a simulation that does the right thing when I explicitly specify pairs of indices:

ts.Fst(sample_sets = samples_list,
      indexes = [(0,2), (0,1)])

outputs

array([[0.12508121, 0.12780193]])

but that gives the following error if I remove the indexes input (and so use the default value of None)

---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-6-fbfb01290311> in <module>
----> 1 ts.Fst(sample_sets = samples_list)

/usr/local/lib/python3.7/site-packages/tskit/trees.py in Fst(self, sample_sets, indexes, windows, mode, span_normalise)
   3332         fst = np.repeat(1.0, np.product(divergences.shape))
   3333         fst.shape = divergences.shape
-> 3334         for i, (u, v) in enumerate(indexes):
   3335             denom = (diversities[:, :, u] + diversities[:, :, v]
   3336                      + 2 * divergences[:, :, i])

TypeError: 'NoneType' object is not iterable
@jeromekelleher jeromekelleher added this to the Python 0.2.0 milestone Jul 2, 2019
@jeromekelleher jeromekelleher added the bug Something isn't working label Jul 2, 2019
@jeromekelleher
Copy link
Member

See also #199 (comment)

@jeromekelleher
Copy link
Member

I was looking into this @petrelharp, and it seems we don't have a lot of test coverage on Fst right now --- we're only testing the "site" version, essentially. Since this is a derived statistic it seems overboard to implement the full thing from scratch like the other stats, but we do need to get some sort of coverage here on "branch" and "node". Have you any suggestions for this?

@petrelharp
Copy link
Contributor

I guess we just need to do some "does this run" sort of checks. I'll have a look.

@petrelharp
Copy link
Contributor

petrelharp commented Aug 19, 2019

Currently our default for indexes=None is indexes=[(0, 1)] for pairwise stats. We can't do this here because users might think that the single Fst value returned is the between-all-pops Fst value. I also don't want to just return the full matrix by default because it's not clear what order it should be in. So, I'm making the user specify.

@jeromekelleher
Copy link
Member

Currently our default for indexes=None is indexes=[(0, 1)] for pairwise stats. We can't do this here because users might think that the single Fst value returned is the between-all-pops Fst value. I also don't want to just return the full matrix by default because it's not clear what order it should be in. So, I'm making the user specify.

I see. What if [(0, 1)] was the default when we have two sample sets, but we raise a ValueError if there's more than 2 sample sets and indexes is None? We want to make the common case easy and simple.

petrelharp added a commit that referenced this issue Aug 20, 2019
removed duplicate test code
@petrelharp
Copy link
Contributor

Oh, that's a good idea.

@petrelharp
Copy link
Contributor

I've done this, although similar defaults could be set for many other statistics, which we should probably do at some point...

@jeromekelleher
Copy link
Member

I've had a change of heart here, and think maybe we should hold off on specifying a default for this---see the comment in #313

@petrelharp
Copy link
Contributor

Whoops; missed this. Let's discuss at #315 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants