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

Chevychase #48

Merged
merged 12 commits into from
Feb 2, 2021
Merged

Chevychase #48

merged 12 commits into from
Feb 2, 2021

Conversation

kmdalton
Copy link
Member

@kmdalton kmdalton commented Feb 2, 2021

This pull request will

  • improve quadrature integration for French Wilson scaling
  • add updated reference data from pymc3
  • add improved tests for posterior parameter estimation

The major point is that we now have a much larger gold standard test data set in the form of Markov chain Monte Carlo integration performed in pymc3. I've ensured that the output of our algorithm stays very close to the MCMC output. I've improved numerical stability and decreased memory requirement by using Gauss-Chebyshev quadrature with degree 100. It is possible we can lower this degree setting if there are performance concerns. Currently, all posterior parameters are within 2.5% of the MCMC values as compared to 6% for the cctbx implementation of the classical algorithm.

I think this implementation constitutes a new state of the art. Two things we could potentially do to improve down the line:

  • optimize the integration window size in order to maximize consistency with MCMC
  • optimize the degree of the approximation
    Both of these things are set quite arbitrarily right now.

Here are some histograms of percent errors between rs and MCMC for the four posterior parameter estimates:

image

image

image

image

@codecov-io
Copy link

codecov-io commented Feb 2, 2021

Codecov Report

Merging #48 (71f10b4) into master (e7b8c8f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #48   +/-   ##
=======================================
  Coverage   99.17%   99.17%           
=======================================
  Files          34       34           
  Lines        1330     1341   +11     
=======================================
+ Hits         1319     1330   +11     
  Misses         11       11           
Flag Coverage Δ
unittests 99.17% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...alspaceship/algorithms/scale_merged_intensities.py 100.00% <100.00%> (ø)
reciprocalspaceship/dataset.py 99.02% <0.00%> (+0.02%) ⬆️

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 e7b8c8f...71f10b4. Read the comment docs.

@JBGreisman JBGreisman added the enhancement Improvement to existing feature label Feb 2, 2021
Copy link
Member

@JBGreisman JBGreisman 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. I'm not crazy about including the standalone plotting script diagnostics.py, but if you find it useful I think its fine. It's at least in the testing > data subdirectory specific to french-wilson so its context is fairly clear.

@kmdalton kmdalton merged commit 22d0cd9 into master Feb 2, 2021
@kmdalton kmdalton deleted the chevychase branch February 2, 2021 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement to existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants