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

[WIP] Implement Proposals.HSlice #45

Draft
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

SaranjeetKaur
Copy link
Contributor

No description provided.

@SaranjeetKaur SaranjeetKaur marked this pull request as draft July 9, 2020 16:37
@codecov
Copy link

codecov bot commented Jul 9, 2020

Codecov Report

Merging #45 into master will decrease coverage by 0.19%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #45      +/-   ##
==========================================
- Coverage   88.23%   88.03%   -0.20%     
==========================================
  Files           9        9              
  Lines         442      443       +1     
==========================================
  Hits          390      390              
- Misses         52       53       +1     
Flag Coverage Δ
#unittests 88.03% <ø> (-0.20%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/proposals/Proposals.jl 93.75% <ø> (ø)
src/bounds/Bounds.jl 80.00% <0.00%> (-3.34%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 19f5273...5478297. Read the comment docs.

@SaranjeetKaur SaranjeetKaur marked this pull request as ready for review July 29, 2020 19:08
Copy link
Collaborator

@mileslucas mileslucas left a comment

Choose a reason for hiding this comment

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

Okay, you need to revert all the whitespace changes you've made to the docstrings and such. Please don't do this again in the future.

src/proposals/Proposals.jl Outdated Show resolved Hide resolved
Copy link
Member

@cpfiffer cpfiffer left a comment

Choose a reason for hiding this comment

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

I can't really speak to the actual numerical outcome because I'm not super familiar with the methods.

It does seem that HSlice is really long and should be cut up a lot. It also looks like there's lots of repeated code, though I think you've already mentioned you're going to factor this stuff out.

Also, this is a general question for this repo, how is the actual accuracy of these proposals being tested? Like if I give it a simple model, does it get the mean/variance right?

src/proposals/Proposals.jl Show resolved Hide resolved
src/proposals/Proposals.jl Show resolved Hide resolved
@cpfiffer
Copy link
Member

Also, all the tests are failing. Definitely a good idea to fix those before we can make any meaningful comments.

@mileslucas
Copy link
Collaborator

mileslucas commented Jul 29, 2020

Also, this is a general question for this repo, how is the actual accuracy of these proposals being tested? Like if I give it a simple model, does it get the mean/variance right?

Right now I'm testing some trivial properties of the proposals, such as "does the proposed point have higher likelihood than the critical value?" Implicitly it also makes sure we don't hit any of those errors related to extremely inefficient sampling (this is all in test/proposals.jl). In addition, they're being used inside the sampling integration tests, which do have analytical outputs we can test against (this is in test/sampling.jl).

Looking at other nested sampling libraries this actually seems like more coverage already. For example, dynesty's tests are almost all integration tests that don't even test all combos of proposals and bounds.

I'd love to get some more granularity to be able to test the corner cases and expectations of the proposals, but this work isn't directly part of my current research so I can't commit as much time to this package to polish off the corners.

@SaranjeetKaur SaranjeetKaur marked this pull request as draft September 1, 2021 15:32
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.

3 participants