Issue 3601 -- astropy.stats.funcs.bootstrap now accepts bootfuncs with multiple outputs - #3628

@ezbc Your PR so far has the changelog entry and some tests which look good, but is missing the actual change to the function. Is it intentionally not implemented yet?

@embray This was not intentional, sorry about that. I must have committed funcs to a different branch.

I believe the changes are now pushed to the correct branch, issue3601, and are now ready to be merged.

@ezbc - Thanks for the contribution. I left a few basic comments for an issue that causes test failures.

Otherwise I'll leave the code review to others who are more familiar with this module.

@ezbc @bsipocz (and @ttshimiz , who created #3601) - Sorry I'm only saying this now (I didn't think about it until I started reviewing this in detail), but I'm not sure this is the best solution to issue #3601 . I think this approach adds unnecessary complication where a reasonably straightforward solution already exists in python: defining a new function. For example, the following works the same as the case here that uses output_index=0:

bootstrap(bootarr, 2, bootfunc=lambda x:spearmanr(x)[0])

I think this might be better because it makes the bootstrap function itself simpler while at the same time allowing other more complicated situations e.g. you want to use a different input (i.e., bootstrap(bootarr, bootfunc=lambda x: func(1, x)).

So that would require no changes to the code... but, @ezbc, most of the work you've done here is adding tests, and we certainly don't want to waste that. So perhaps the think to do is to back out the changes to the code, but then adapt the tests to use the lambda function approach, and also add an example to the documentation showing this use case? @ttshimiz, would you consider that an acceptable solution to #3601 ?

(@ezbc - just in case, no need to worry that this will affect your GSoC application even if we make the changes - you'll still get credit for the effort you put in here as far as the application is concerned no matter how this PR actually turns out, I promise!)

@ezbc, @bsipocz, @eteq Yeah that would definitely be an acceptable solution to me. Examples and an explanation in the documentation would really help to show people how to use bootstrap with functions that have multiple inputs and outputs. It didn't even occur to me to use lambda to just define a new function that only returned the first output of spearmanr. In my view, simpler is always better. Thanks for addressing this issue!

Thanks for input @embray! Good thinking. I'll try to work in your suggestion into the documentation and testing this week.

@ezbc I think you meant to thank @eteq for his input. Yes, there are two Eriks in this thread :) It's easy to remember because the other one's username ends in q which is sort of odd :)

Aha, good suggestion @embray.

Thanks for input @eteq! I'll work on the suggestion today.

@eteq, one question. I'm not sure when this case may arise, but perhaps someone would want to bootstrap data, retaining two values from the output of bootfunc. I do not know how to do this using lambda functions besides running the bootstrap function twice with the same RNG seed, retaining a different output from the bootfunc.

Can you think of a simpler way to retain two values from bootfunc, or is this even necessary? If using a seed with two bootstrap runs is necessary, should we include an optional argument in the bootstrap function to accept a seed number? Otherwise, I suppose I could just include an example in the documentation of using astropy.utils.misc.NumpyRNGContext to supply a seed to bootfunc.

Thanks!

@ezbc - Ah, I hadn't thought of that. Not sure how often it would be useful, but it might well be!

So this leads me to think the best thing is a "hybrid" solution: modify the function so that it still has no new inputs, but the output can be multi-dimensional. So if bootfunc returns a single value, have it behave just as it does now, but if it's multiple values, then the output is an NxM array where N is the number of resamples and M is the number of return values that bootfunc yields. I'd still advocate the lambda function example be included, but with that change, an alternative solution is bootstrap(bootarr, 2, bootfunc=spearmanr)[:, 0], which is even simpler. This won't work if bootfunc gives back something irregular (i.e., if the first return value is an array and the second is a scalar), but then the situation is probably complex enough that the user should just do it themselves...

@ezbc - do you think you'll be able to get to doing this?

@eteq - My apologies, lots on the plate in the past month, and now I'm going on holiday. I will work on this the last week of June if the issue is still open.

@eteq - I made the changes we discussed:

• reverted the bootstrap function to the original, with the added ability for multiple outputs
• added examples to the bootstrap function
• changed testing to include single output and multiple output tests of bootfuncs

Sorry this took so long! Let me know if there's something I should change/add.

@ezbc - could you address the test failures on Travis?

@astrofrog - I believe I addressed these issues.

Would you be able to point me to a resource where I could learn to make these build checks locally myself, to avoid such issues in the future? Thanks!

@ezbc - there is no easy way to run all the configurations, but you can run the tests with

python setup.py test

What I normally do is have a Python 2 and Python 3 environment in Anaconda, and I can run the tests in both of these. But no one really tests all the configurations before pushing a pull request. If you see Travis failures that are not obvious, you can use conda to set up a similar environment.

Thank you, I'll try to use Anaconda in this way in the future.

On Thu, Oct 8, 2015 at 4:18 AM, Thomas Robitaille [email protected]
wrote:

@ezbc https://github.com/ezbc - there is no easy way to run all the
configurations, but you can run the tests with

python setup.py test

What I normally do is have a Python 2 and Python 3 environment in
Anaconda, and I can run the tests in both of these. But no one really tests
all the configurations before pushing a pull request. If you see Travis
failures that are not obvious, you can use conda to set up a similar
environment.

—
Reply to this email directly or view it on GitHub
#3628 (comment).

Elijah Bernstein-Cooper
[email protected]
Department of Astronomy
University of Wisconsin - Madison

So all set for a final review? I'm happy to give it one if that is the case.

I believe so. Let me know if there's anything which needs editing.

On Fri, Oct 16, 2015 at 5:06 AM, Steve Crawford [email protected]
wrote:

So all set for a final review? I'm happy to give it one if that is the
case.

—
Reply to this email directly or view it on GitHub
#3628 (comment).

Elijah Bernstein-Cooper
[email protected]
Department of Astronomy
University of Wisconsin - Madison

One relatively small edit and otherwise, this looks good to merge. Thanks for the contribution @ezbc !

Sorry about that, it's my first time contributing. I'll follow the guidelines in the future.

Let me know if I missed something.

@ezbc - Sure, no problem, our comments were also misleading.

Thanks for the contribution. @crawfordsm the coverage fail is back again, but we saw it must be unrelated (as we saw it pass before the rebase). Also appveyor has an unrelated issue, and it passed before, too.

@astrofrog Just checking, it is okay to merge this as the failures are unrelated to the pull request?

@crawfordsm - I've restarted AppVeyor, just to be sure

Merging-- thanks @ezbc for the contribution and everyone else for the review!

Thank you all for your patience!

On Mon, Nov 9, 2015 at 9:58 PM, Steve Crawford [email protected]
wrote:

Merged #3628 #3628.

—
Reply to this email directly or view it on GitHub
#3628 (comment).

Elijah Bernstein-Cooper
[email protected]
Department of Astronomy
University of Wisconsin - Madison

@crawfordsm @ezbc - can someone clarify for me what the version is for this PR? It looks to me like it came in after 1.1 feature freeze, which means it should be v1.2 . But it has the v1.1 milestone and appears in the 1.1 changelog. Is that an oversight?

I'm going to re-milestone it as 1.2 assuming that's the case (and there will be a reckoning later to make the master changelog consistent with what's actually in v1.1, but if I'm misunderstanding please let me know ASAP.

@eteq It did only get merged in after the 1.1 feature freeze so unfortunately due to various reasons it was not in time for 1.1. I think I mention up earlier in the thread it the change log should have been moved to the 1.2 version. So since this can't go into the 1.1, then it should be 1.2. Let me know if there is something I can do to help with the change log as I must have missed that in the final review.

Although @embray looks to added the 1.1 milestone 8 days ago, so perhaps he should comment as well.

I don't really care, but the changelog entry was made for v1.1 so that's why I put it there.

OK, thanks for clearing that up - in 8869e8d I moved it to the right changelog section directly in master.