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

Rewrite EnergyDispersion class #351

Merged
merged 14 commits into from Sep 18, 2015
Merged

Rewrite EnergyDispersion class #351

merged 14 commits into from Sep 18, 2015

Conversation

@joleroi
Copy link
Contributor

@joleroi joleroi commented Sep 9, 2015

This PR updates the EnergyDispersion class that wraps the OGIP RMF format:
Changes include:

  • Fits output transferred from external np_to_rm function to class member and rewritten (use numpy instead of for loops)
  • Energy handling updated in order to use gammapy.spectrum.energy (including some updates in the Energy classes)

NOT included

  • Update RMF reader (from_hdu_list)
  • Updates of plot routines
  • Test for EnergyDispersion2D -> EnergyDispersion
@cdeil cdeil added the feature label Sep 9, 2015
@cdeil cdeil added this to the 0.4 milestone Sep 9, 2015
@cdeil
Copy link
Member

@cdeil cdeil commented Sep 9, 2015

Let me know if you have any questions or when I should look at something or try it out ...

@cdeil
Copy link
Member

@cdeil cdeil commented Sep 9, 2015

@kingj90 – I hope #352 won't conflict with this PR!
Apologies ... I did touch the effective area and energy resolution classes again.

@joleroi
Copy link
Contributor Author

@joleroi joleroi commented Sep 15, 2015

@cdeil
As discussed I wont touch any of the plot routines, only fits/io and energy handling. so there should not be too much interference, I will have a ready to merge version of the IRF classes until tomorrow

@joleroi joleroi changed the title WIP: Towards a command-line tool for spectral fitting WIP: Rewrite most parts of EnergyDispersion class (Fits I/O) Sep 16, 2015
@joleroi
Copy link
Contributor Author

@joleroi joleroi commented Sep 16, 2015

This is becoming rather large again, I suggest we leave it for this PR (after adding some tests)

@cdeil
Copy link
Member

@cdeil cdeil commented Sep 16, 2015

Is this sphinx build error unrelated or was it introduced in this PR?
https://travis-ci.org/gammapy/gammapy/jobs/80646680#L1642
Can you check locally with python setup.py build_sphinx -l?

Are you still working on this or should I review now?

e_reco = EnergyBounds.equal_log_spacing(1,10,5,'TeV')
rmf = edisp.to_energy_dispersion(e_reco, offset)

actual = rmf._pdf_matrix[5]

This comment has been minimized.

@cdeil

cdeil Sep 16, 2015
Member

Ideally tests shouldn't have to access private things like _pdf_matrix here.
Maybe it makes sense to expose this as a property or make it a public member (i.e. remove the underscore)?

@joleroi
Copy link
Contributor Author

@joleroi joleroi commented Sep 16, 2015

I am still working on this, since I keep discovering bugs while adding the tests

@cdeil
Copy link
Member

@cdeil cdeil commented Sep 17, 2015

I think the reason for the travis-ci fail was the matplotlib intersphinx fetch timeout.

After re-starting, travis-ci now shows green light on this build:
https://travis-ci.org/gammapy/gammapy/jobs/80798983

@cdeil
Copy link
Member

@cdeil cdeil commented Sep 17, 2015

Can you please edit the description of this PR at the top, adding two sentences what's done here and probably removing the task list?

@joleroi
Copy link
Contributor Author

@joleroi joleroi commented Sep 17, 2015

I will clean up a little bit and the ask for comments

@cdeil
Copy link
Member

@cdeil cdeil commented Sep 17, 2015

I looked over this 5 minutes ago and it looks good.

I don't have time to test this or review this today, so my advice to be either to merge as-is,
or to look at the code coverage for these parts of the code and add tests for things that are completely untested at the moment.

In any case ... feel free to push the "Merge pull request" button yourself when ready!

@joleroi joleroi added cleanup and removed feature labels Sep 17, 2015
joleroi pushed a commit that referenced this pull request Sep 18, 2015
Rewrite most parts of EnergyDispersion class (Fits I/O)
@joleroi joleroi merged commit 3035ae4 into gammapy:master Sep 18, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@joleroi joleroi deleted the joleroi:spectralfit branch Sep 18, 2015
@joleroi joleroi mentioned this pull request Sep 18, 2015
4 of 4 tasks complete
joleroi added a commit that referenced this pull request Sep 18, 2015
dlennarz pushed a commit to dlennarz/gammapy that referenced this pull request Oct 11, 2015
@cdeil cdeil changed the title WIP: Rewrite most parts of EnergyDispersion class (Fits I/O) Rewrite EnergyDispersion class Oct 31, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants