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

Random sampler analysis class #273

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from
Open

Conversation

wedeling
Copy link
Collaborator

@wedeling wedeling commented Oct 1, 2020

Made the computation of the confidence intervals more efficient, by vectorizing the for loop over the number of bootstrap samples.

@lgtm-com
Copy link

lgtm-com bot commented Oct 1, 2020

This pull request introduces 1 alert when merging fcff1c3 into 6ab8e9e - view on LGTM.com

new alerts:

  • 1 for Unused local variable

Copy link
Collaborator

@orbitfold orbitfold left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@orbitfold
Copy link
Collaborator

oh you will need to change test_mc_analysis.py, the [0] indexing just needs to be removed i think. since this changes how results are returned?

@wedeling
Copy link
Collaborator Author

wedeling commented Oct 1, 2020

It shouldn't matter I think. I'll have a look

@orbitfold
Copy link
Collaborator

>       assert(s1_conf['x1']['low'] == pytest.approx(0.14387035, 0.01))
E       assert 0.19341946180114922 == 0.14387035 ± 1.4e-03
E        +  where 0.14387035 ± 1.4e-03 = <function approx at 0x7f12acd2fa60>(0.14387035, 0.01)
E        +    where <function approx at 0x7f12acd2fa60> = pytest.approx

Is there some deeper issue here or just because of the changed order of the calls to the random number generator?

@lgtm-com
Copy link

lgtm-com bot commented Oct 1, 2020

This pull request introduces 1 alert when merging 8fa541a into 6ab8e9e - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@DavidPCoster
Copy link
Collaborator

@wedeling -- should this be fixed and merged? Or discarded? Work on this started more than 3 years ago ...

@wedeling
Copy link
Collaborator Author

wedeling commented Dec 5, 2023

@DavidPCoster Not sure, it's got quite some work in it, but's it's been 3 years, there might be conflicts with the current version. Also, the HDF5 collator probably deserves it's own PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants