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

Add gammapy/utils/fitting/sampling.py #2289

Merged
merged 12 commits into from Jul 22, 2019
Merged

Add gammapy/utils/fitting/sampling.py #2289

merged 12 commits into from Jul 22, 2019

Conversation

@facero
Copy link
Contributor

@facero facero commented Jul 16, 2019

As discussed in PIG14 and in #2255, this is a first attempt to move helpers function of mcmc_sampling.ipynb to utils.fitting.

Naming, unit tests, and comments are welcome.
With this change the mcmc can now be run in one line :
sampler = run_mcmc(dataset, nwalkers=6, nrun=150)
this could be used in a 2nd step to add mcmc as a fitting backend.
Plotting functions have also been added such as plot_trace(sampler, dataset) & plot_corner(sampler, dataset, nburn=50)

@facero
Copy link
Contributor Author

@facero facero commented Jul 16, 2019

I've deleted the previous PR toa void modifying the astropy_helpers file.
I've updated the mcmc notebook and can commit it as part of this PR if this is the best way.
I guess it would be best to avoid having a test for the run_mcmc module as this will add unwanted exec time in tests.
Maybe on the uniform priors and model_to_par stuff ?
Any advice ?

@cdeil cdeil self-assigned this Jul 17, 2019
@cdeil cdeil added the feature label Jul 17, 2019
@cdeil cdeil added this to the 0.13 milestone Jul 17, 2019
@cdeil cdeil added this to In progress in gammapy.modeling via automation Jul 17, 2019
@cdeil
Copy link
Member

@cdeil cdeil commented Jul 17, 2019

I've updated the mcmc notebook and can commit it as part of this PR if this is the best way.

Please do add the notebook update here in this PR.

I guess it would be best to avoid having a test for the run_mcmc module as this will add unwanted exec time in tests. Maybe on the uniform priors and model_to_par stuff ? Any advice ?

It's difficult to add useful tests in this case, no doubt.

The tests have to be fast: 1 second is good, up to 10 sec is acceptable.
So at least in this case, the tests can't run a good science analysis.

The purpose of the tests is to allow us to refactor and improve things in Gammapy, without fear that we break functionality. So what's needed is a test that executes the code once and that has some assert at the end. That's the difference wrt. the notebook: in this case you do have code that executes this once, but it's inconvenient & slow for devs to execute as they work on this code, and it doesn't have an assert at the end, one would have to manually note and compare output before and after.

So can you find a simple testcase to execute this code once? I think one "functional" test would be good, even if it takes 10 or 20 lines to set up. In this case, unit tests, just calling each function with some number as input doesn't help much, it doesn't show that we have a working feature. Maybe you can just take 10 samples and assert on their mean and std, or on the first and last sample for some parameter?

Copy link
Member

@cdeil cdeil left a comment

@facero - Some small code & docstring suggestions inline.

gammapy/utils/fitting/sampling.py Outdated Show resolved Hide resolved
gammapy/utils/fitting/sampling.py Outdated Show resolved Hide resolved
gammapy/utils/fitting/sampling.py Outdated Show resolved Hide resolved
gammapy/utils/fitting/sampling.py Outdated Show resolved Hide resolved
gammapy/utils/fitting/sampling.py Outdated Show resolved Hide resolved
gammapy/utils/fitting/sampling.py Show resolved Hide resolved
gammapy/utils/fitting/sampling.py Outdated Show resolved Hide resolved
gammapy/utils/fitting/sampling.py Show resolved Hide resolved
@facero
Copy link
Contributor Author

@facero facero commented Jul 17, 2019

the notebook has been updated and the logging in the notebook works.
Still need to write a few unit tests

Copy link
Member

@cdeil cdeil left a comment

@facero - Thanks! We should merge this in, with or without automated tests, release is scheduled for Monday. Let me know when to press the breen button...

gammapy/utils/fitting/sampling.py Outdated Show resolved Hide resolved
@facero
Copy link
Contributor Author

@facero facero commented Jul 18, 2019

Fixed the last small issue with logging. You can go ahead and merge whenever you want.

@cdeil cdeil dismissed their stale review Jul 19, 2019

outdated

@cdeil
Copy link
Member

@cdeil cdeil commented Jul 19, 2019

@facero - I've done some small changed in 210d18e .

We should merge without tests, right?
Or did you manage to think of some test?

My main suggestion here would be to only use parameter.value in the MCMC code, never parameter.factor.

The value = factor * scale split was only introduced to avoid numerical issues with the covariance matrix. Most optimisers don't have those, they do this split internally for operations if needed for numerical stability. I'm not sure if we want to keep it at all.

But for new things like MCMC fitting I think we should just use parameter.value, unless it's known that emcee doesn't work if you have parameters that are e.g. [1, 1e-10, 1e-20]. That's one thing why a test with an assert_allclose on a reference value would be super valuable - we could just change from factor to value in the emcee interface, and see if the outcome is almost identical.

I'll wait for CI to pass. @facero - Do you want to do any further edit here today? Or should I merge this in and make a reminder issue to add tests and try to change factor -> value later?

@facero
Copy link
Contributor Author

@facero facero commented Jul 19, 2019

I won't have today to work on it more so you can go ahead.
the value or factor question is a valid one, I will have to do some more tests.
I think I used factor as it wasn't working great if the values were super small (1e-12) but need to check again.

@cdeil
Copy link
Member

@cdeil cdeil commented Jul 22, 2019

Looks like there's one more issue:
https://travis-ci.org/gammapy/gammapy/jobs/561808362#L1345

I'll fix that up now and then merge.

@cdeil
Copy link
Member

@cdeil cdeil commented Jul 22, 2019

Fixed in 8bc6381 . Merging now.

cdeil
cdeil approved these changes Jul 22, 2019
@cdeil cdeil merged commit 155fd2f into gammapy:master Jul 22, 2019
6 of 9 checks passed
gammapy.modeling automation moved this from In progress to Done Jul 22, 2019
@facero facero deleted the mcmc_utils branch Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants