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

Improve gammapy.astro code and tests #2192

Merged
merged 11 commits into from May 29, 2019
Merged

Improve gammapy.astro code and tests #2192

merged 11 commits into from May 29, 2019

Conversation

@cdeil
Copy link
Member

@cdeil cdeil commented May 29, 2019

This PR started with the goal to replace gammapy/utils/coordinates/other.py (that code is older than Astropy and somehow survived until now) with use of astropy.coordinates, continuing #2185 from yesterday. This PR implements many things noted in #977, but I won't do all here (e.g. not write a high-level tutorial notebook).

The only caller of that code is gammapy.astro.population, in spatial.py and simulate.py.

I quickly started noticing bugs and issues there, so by now this PR is mostly a mixed bag of improvements in gammapy.astro code, docstrings and tests.

  • Remove add_observed_source_parameters - it was buggy, not tested, and not really useful
  • Improve make_catalog_random_positions_cube and make_catalog_random_positions_sphere
  • Add tests with asserts on all columns and units from the functions in simulate.py
  • Remove PWN.luminosity_tev and L_PWN in simulate.py. It was the energy dumped by the pulsar in erg, but there was no radiation model or timescale and thus no luminosity to give.
  • Remove Pulsar.luminosity_tev (not useful, just callse Pulsar.luminosity_spindown and multiplies with a number the user passes in.
  • Change Pulsar attribute from logB to B to avoid ambiguity concerning log_10 or log_e.
  • Change from validate_physical_type to pass through Quantity, like we do everywhere else in Gammapy
  • Put method description in the main part of the docstring, instead of always having it in a Notes section. That's IMO a bit shorter, easier to grok, and more consistent with the rest of Gammapy / Astropy.

The diff here is already ~ 500 lines, hard to review. I'm merging this now, and will send a follow-up PR to rewrite gammapy/utils/coordinates/other.py and the coords code in gammapy.astro.population.

@cdeil cdeil added this to the 0.13 milestone May 29, 2019
@cdeil cdeil self-assigned this May 29, 2019
@cdeil cdeil added this to In progress in gammapy.astro via automation May 29, 2019
@cdeil cdeil merged commit f2cbf87 into gammapy:master May 29, 2019
5 of 9 checks passed
gammapy.astro automation moved this from In progress to Done May 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
gammapy.astro
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

1 participant