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

Fixed the way random walker starts are picked over a boundary #89

Merged
merged 6 commits into from Feb 16, 2023

Conversation

KriSun95
Copy link
Collaborator

@KriSun95 KriSun95 commented Dec 6, 2022

The way I did this in the first place was stupid. Originally the parameter space bounds were multiplied by 100 and then I choose random integer values (why do I want to make everything an integer!?) within the new scaled bounds. This is obviously not right.

The new way is done as L+(U-L)*R, where L and R are the lower and upper bounds for the parameter space and R is a random value between 0 and 1. This should now give values that make sense no matter the bounds.

Copy link
Collaborator

@samaloney samaloney left a comment

Choose a reason for hiding this comment

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

Thanks, nice catch. I think a test calling _boundary_spread with some vales and comparing to expected would be really nice.

changelog/89.bugfix.rst Outdated Show resolved Hide resolved
@settwi
Copy link
Contributor

settwi commented Jan 13, 2023

I think this approach still assumes that the parameter bounds are close to the best value.

For things like emission measure I've often made the bounds very large (like 1e-5 to 1e20). The 'mag_order' approach makes more sense for starting close to the minimizer parameters.

Why not make 'mag_order' the default?

@codecov-commenter
Copy link

codecov-commenter commented Feb 16, 2023

Codecov Report

Base: 54.63% // Head: 54.86% // Increases project coverage by +0.22% 🎉

Coverage data is based on head (6f842ca) compared to base (1a9e105).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #89      +/-   ##
==========================================
+ Coverage   54.63%   54.86%   +0.22%     
==========================================
  Files          19       19              
  Lines        3115     3115              
==========================================
+ Hits         1702     1709       +7     
+ Misses       1413     1406       -7     
Impacted Files Coverage Δ
sunxspex/sunxspex_fitting/fitter.py 54.10% <100.00%> (+0.51%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@samaloney samaloney merged commit 707fc57 into sunpy:master Feb 16, 2023
@KriSun95 KriSun95 deleted the kris-mcmcSpread branch March 6, 2023 22:24
settwi pushed a commit to settwi/sunkit-spex that referenced this pull request Sep 18, 2023
* Fixed the way random walker starts are picked over a boundary for an MCMC run.
* Added test for the method that starts walkers off randomly over the boundary for MCMC analysis.
* Now using numpy for the largest 32-bit integer rather than, mistakeny hard-coding the wrong one.
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.

None yet

4 participants