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.

Sign up for GitHub

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

Jump to bottom

Merge vonmises into stats pyx #5944

Merged
merged 2 commits into from
Mar 28, 2016
Merged

Merge vonmises into stats pyx #5944

merged 2 commits into from
Mar 28, 2016

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Mar 8, 2016

Suggested in #5936, only makes sense if that PR is merged first.

@rgommers rgommers added scipy.stats maintenance Items related to regular maintenance tasks labels Mar 13, 2016
@rgommers
Copy link
Member

One test failure:

======================================================================
FAIL: test_distributions.test_all_distributions('vonmises', (101,), 0.01)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/virtualenv/python3.5.0/lib/python3.5/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/home/travis/virtualenv/python3.5.0/lib/python3.5/site-packages/scipy/stats/tests/test_distributions.py", line 65, in check_distribution
    "; alpha = " + str(alpha) + "\nargs = " + str(args))
  File "/home/travis/virtualenv/python3.5.0/lib/python3.5/site-packages/numpy/testing/utils.py", line 71, in assert_
    raise AssertionError(smsg)
AssertionError: D = 0.271825589958; pval = 0.0; alpha = 0.01
args = (101,)

@rgommers
Copy link
Member

merging the files make sense to me

@anntzer anntzer force-pushed the merge-vonmises-into-stats-pyx branch from e34a80d to 8922926 Compare March 16, 2016 05:44
@anntzer
Copy link
Contributor Author

anntzer commented Mar 16, 2016

Should be fixed now. Also rebased on the new #5936.

@larsoner
Copy link
Member

Travis is unhappy about some minor doc issues:

https://travis-ci.org/scipy/scipy/jobs/116304847#L3460

@anntzer anntzer force-pushed the merge-vonmises-into-stats-pyx branch from 8922926 to 38f65ca Compare March 16, 2016 18:14
@anntzer
Copy link
Contributor Author

anntzer commented Mar 16, 2016

fixed

ev-br
ev-br reviewed Mar 24, 2016
View reviewed changes
scipy/stats/_stats.pyx
for i in range(len(temp)):
p = <int>(1 + a1 + a2 * temp_ks[i] - a3 / (temp_ks[i] + a4))
temp[i] = von_mises_cdf_series(temp_ks[i], temp_xs[i], p)
temp[i] = 0 if temp[i] < 0 else 1 if temp[i] > 1 else temp[i]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoa. Can't believe this is legal. I had to read it out loud, then again, and then consult the red-colored part of the diff. This is very cool, but please have some mercy for us mere mortals.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this actually reads quite "intuitively" (as if it was English)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, you can probably attribute my confusion to ESL.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, do you want me to change this? I think it's more readable than min(max(temp[i], 0), 1)...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it were my PR, I would just write out two nested if-then-else clauses. Which is not cute etc. But this is your PR, and I won't hold it for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll just leave it as it is then, unless someone else objects.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although this is unlikely to be a bottleneck, it still looks like gratuitiously invoking the Python runtime...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pv
Copy link
Member

pv commented Mar 28, 2016

Needs rebase, the other PR was merged.

@anntzer anntzer force-pushed the merge-vonmises-into-stats-pyx branch from 38f65ca to 9ca3d3f Compare March 28, 2016 16:35
@anntzer
Copy link
Contributor Author

anntzer commented Mar 28, 2016

Done.

@codecov-io
Copy link 

@@            master   #5944   diff @@
======================================
  Files          238     238       
  Stmts        43822   43822       
  Branches      8215    8215       
  Methods          0       0       
======================================
  Hit          34251   34251       
  Partial       2603    2603       
  Missed        6968    6968

Review entire Coverage Diff as of e490549

Powered by Codecov. Updated on successful CI builds.

@ev-br
Copy link
Member

ev-br commented Mar 28, 2016

Ok, merging. Thanks @anntzer

@ev-br ev-br merged commit 47efe09 into scipy:master Mar 28, 2016
@ev-br ev-br added this to the 0.18.0 milestone Mar 28, 2016
@anntzer anntzer deleted the merge-vonmises-into-stats-pyx branch March 28, 2016 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
maintenance Items related to regular maintenance tasks scipy.stats
Projects
None yet
Milestone
0.18.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@anntzer @rgommers @larsoner @pv @codecov-io @ev-br