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 iterative kernel background estimator #186

Merged
merged 6 commits into from Sep 17, 2014

Conversation

Projects
None yet
2 participants
@ellisowen
Contributor

ellisowen commented Aug 26, 2014

Made a new PR as couldn't push to the old one: #173

Still in progress. To do:

  • Add unit tests
  • Finish tutorial
  • Include initial image and mask image additionally in the tutorial
  • Clean up code formatting
  • Add docstrings
    (Anything else?)

Please comment if you see anything to be done or changed.

Show outdated Hide outdated gammapy/background/kernel.py Outdated
Show outdated Hide outdated gammapy/background/kernel.py Outdated
Show outdated Hide outdated gammapy/background/kernel.py Outdated
Show outdated Hide outdated gammapy/background/kernel.py Outdated

@cdeil cdeil added this to the 0.2 milestone Aug 26, 2014

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Aug 26, 2014

Member

Thanks for doing this!
I'm closing #173 now.

The task list at the top looks good to me ... start by adding tests.
Does the tutorial example already work?

I'll review this in the coming days...

Member

cdeil commented Aug 26, 2014

Thanks for doing this!
I'm closing #173 now.

The task list at the top looks good to me ... start by adding tests.
Does the tutorial example already work?

I'll review this in the coming days...

@ellisowen

This comment has been minimized.

Show comment
Hide comment
@ellisowen

ellisowen Aug 26, 2014

Contributor

Currently the tutorial example works, but I'm yet to determine whether the result is OK. I think this will become clear as I write the tests (I don't really trust the termination step yet).

I will hopefully have a complete version ready for review by tomorrow morning.

Contributor

ellisowen commented Aug 26, 2014

Currently the tutorial example works, but I'm yet to determine whether the result is OK. I think this will become clear as I write the tests (I don't really trust the termination step yet).

I will hopefully have a complete version ready for review by tomorrow morning.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Aug 26, 2014

Member

As a test-case I'd recommend very small arrays and kernels ... something where one can work out in the head what the answer should be.
E.g. you could use box kernels (e.g. 3x1 for signal and 5x3 for background and as input image something constant (e.g. value 42) with a single pixel with a source (e.g. value 1000).
Then the estimated background should be 42 and the mask should be 3x1 pixels centered on the source.

Member

cdeil commented Aug 26, 2014

As a test-case I'd recommend very small arrays and kernels ... something where one can work out in the head what the answer should be.
E.g. you could use box kernels (e.g. 3x1 for signal and 5x3 for background and as input image something constant (e.g. value 42) with a single pixel with a source (e.g. value 1000).
Then the estimated background should be 42 and the mask should be 3x1 pixels centered on the source.

@ellisowen

This comment has been minimized.

Show comment
Hide comment
@ellisowen

ellisowen Aug 27, 2014

Contributor

OK, I think this is ready for a review now

Contributor

ellisowen commented Aug 27, 2014

OK, I think this is ready for a review now

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Aug 27, 2014

Member

Can you change the tutorial so that you only run the algorithm once?
(you could either create all plots in panels, or use the continue option of the plot directive)

Currently the image has lots of empty space at the top and bottom?
And the mask is not really a great example:
image

How about using this plot that actually illustrates the method in this tutorial?
http://nbviewer.ipython.org/github/gammapy/gammapy-extra/blob/master/notebooks/plot_results.ipynb
(alternatively, update the notebooks to import the class you add here from Gammapy: http://nbviewer.ipython.org/github/gammapy/gammapy-extra/blob/master/notebooks/Index.ipynb#Alphabetical-list-of-notebooks)

Member

cdeil commented Aug 27, 2014

Can you change the tutorial so that you only run the algorithm once?
(you could either create all plots in panels, or use the continue option of the plot directive)

Currently the image has lots of empty space at the top and bottom?
And the mask is not really a great example:
image

How about using this plot that actually illustrates the method in this tutorial?
http://nbviewer.ipython.org/github/gammapy/gammapy-extra/blob/master/notebooks/plot_results.ipynb
(alternatively, update the notebooks to import the class you add here from Gammapy: http://nbviewer.ipython.org/github/gammapy/gammapy-extra/blob/master/notebooks/Index.ipynb#Alphabetical-list-of-notebooks)

Show outdated Hide outdated gammapy/background/kernel.py Outdated
Show outdated Hide outdated gammapy/background/kernel.py Outdated
Show outdated Hide outdated gammapy/background/kernel.py Outdated
Show outdated Hide outdated gammapy/background/kernel.py Outdated
Show outdated Hide outdated gammapy/background/kernel.py Outdated
Show outdated Hide outdated gammapy/image/utils.py Outdated
Show outdated Hide outdated gammapy/background/maps.py Outdated
@ellisowen

This comment has been minimized.

Show comment
Hide comment
@ellisowen

ellisowen Aug 28, 2014

Contributor

OK, I made the changes you suggested, but left a few questions/comments in line. I also introduced some accidental change in gammapy.image.utils, possibly though not rebasing on master or something. I'll fix this in the next round of reviews.

Contributor

ellisowen commented Aug 28, 2014

OK, I made the changes you suggested, but left a few questions/comments in line. I also introduced some accidental change in gammapy.image.utils, possibly though not rebasing on master or something. I'll fix this in the next round of reviews.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Aug 28, 2014

Member

I've left some comments inline ... let me know if you have any further questions or when this is ready for final review.

Member

cdeil commented Aug 28, 2014

I've left some comments inline ... let me know if you have any further questions or when this is ready for final review.

Show outdated Hide outdated gammapy/background/kernel.py Outdated
Show outdated Hide outdated gammapy/background/kernel.py Outdated
@ellisowen

This comment has been minimized.

Show comment
Hide comment
@ellisowen

ellisowen Sep 15, 2014

Contributor

I still need to make changes to the tutorial as we discussed (and add the examples from the proceeding to gammapy-extra), but the main code in kernel.py and tests in test_kernel.py are ready for review.

Contributor

ellisowen commented Sep 15, 2014

I still need to make changes to the tutorial as we discussed (and add the examples from the proceeding to gammapy-extra), but the main code in kernel.py and tests in test_kernel.py are ready for review.

@ellisowen

This comment has been minimized.

Show comment
Hide comment
@ellisowen

ellisowen Sep 16, 2014

Contributor

This is now ready for review.

I will need to update the URL in the tutorial when the gammapy-extra PR is merged (currently this is the cause of the Travis CI failure)

Contributor

ellisowen commented Sep 16, 2014

This is now ready for review.

I will need to update the URL in the tutorial when the gammapy-extra PR is merged (currently this is the cause of the Travis CI failure)

@cdeil cdeil referenced this pull request Sep 16, 2014

Closed

Closed: Add kernel background estimation #173

0 of 2 tasks complete
@ellisowen

This comment has been minimized.

Show comment
Hide comment
@ellisowen

ellisowen Sep 17, 2014

Contributor

This addresses your comments from earlier today...

But sphinx can't build the docs (hence Travis CI failure) because of the iterative plotting in the background tutorial, which isn't available with sphinx apparently. The script works fine when it's just run... I'm not sure what the best way forward is here?

Not urgent anyway, I can just use the branch in it's current state to get the results for the proceeding.

Contributor

ellisowen commented Sep 17, 2014

This addresses your comments from earlier today...

But sphinx can't build the docs (hence Travis CI failure) because of the iterative plotting in the background tutorial, which isn't available with sphinx apparently. The script works fine when it's just run... I'm not sure what the best way forward is here?

Not urgent anyway, I can just use the branch in it's current state to get the results for the proceeding.

Show outdated Hide outdated docs/tutorials/background/index.rst Outdated
@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Sep 17, 2014

Member

This is the current result image:

image

Can you find a way to have less empty area between the panels (and use that space to show the images a bit larger)?

Member

cdeil commented Sep 17, 2014

This is the current result image:

image

Can you find a way to have less empty area between the panels (and use that space to show the images a bit larger)?

Show outdated Hide outdated gammapy/background/kernel.py Outdated
Show outdated Hide outdated gammapy/background/kernel.py Outdated
Show outdated Hide outdated gammapy/background/kernel.py Outdated
Show outdated Hide outdated gammapy/background/kernel.py Outdated
@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Sep 17, 2014

Member

I've left a few inline comments below.

Once you address those this is ready to be merged.
Please clean up the commit history a bit.

If you have time, adding a test that calls save_files would be nice, but it's not super important.

Member

cdeil commented Sep 17, 2014

I've left a few inline comments below.

Once you address those this is ready to be merged.
Please clean up the commit history a bit.

If you have time, adding a test that calls save_files would be nice, but it's not super important.

@ellisowen

This comment has been minimized.

Show comment
Hide comment
@ellisowen

ellisowen Sep 17, 2014

Contributor

OK, I addressed your comments.

I wasn't able to find a way to easily decrease white space around the panels of the iterations image using matplotlib. I can improve it a bit by cropping the images, but then the contours which clearly show mask dilation are lost, so I don't think this is worth doing?

Also the test test_save_files already calls save_files.

Contributor

ellisowen commented Sep 17, 2014

OK, I addressed your comments.

I wasn't able to find a way to easily decrease white space around the panels of the iterations image using matplotlib. I can improve it a bit by cropping the images, but then the contours which clearly show mask dilation are lost, so I don't think this is worth doing?

Also the test test_save_files already calls save_files.

@ellisowen

This comment has been minimized.

Show comment
Hide comment
@ellisowen

ellisowen Sep 17, 2014

Contributor

I just noticed a test is missing - will add this now

Contributor

ellisowen commented Sep 17, 2014

I just noticed a test is missing - will add this now

@ellisowen

This comment has been minimized.

Show comment
Hide comment
@ellisowen

ellisowen Sep 17, 2014

Contributor

OK, this should now be ready!

Contributor

ellisowen commented Sep 17, 2014

OK, this should now be ready!

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Sep 17, 2014

Member

Thanks!

Member

cdeil commented Sep 17, 2014

Thanks!

cdeil added a commit that referenced this pull request Sep 17, 2014

Merge pull request #186 from ellisowen/iterative-kernel-background-es…
…timator

Iterative kernel background estimator

@cdeil cdeil merged commit da02236 into gammapy:master Sep 17, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@cdeil cdeil changed the title from Iterative kernel background estimator to Add iterative kernel background estimator Apr 8, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment