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

Move powerlaw utility functions to separate namespace #599

Merged
merged 1 commit into from Jul 1, 2016

Conversation

Projects
None yet
3 participants
@cdeil
Member

cdeil commented Jun 30, 2016

I'd like to propose that the functions from gammapy.spectrum.powerlaw are moved to a separate namespace.

Currently those functions are part of the main gammapy.spectrum namespace and IMO it's a mess to find something.

So I'd like to propose to either have those functions in

  • a sub-module gammapy.spectrum.powerlaw or
  • a class gammapy.spectrum.PowerLawUtils, i.e. use a class to group the functions and have them as staticmethod.

I don't think that we should add all those functions to the gammapy.spectrum.PowerLaw class, but that is an option as well. Power-law is special, everything is available analytically and it's usually the building block for a lot of other code (like numerical integration in energy).

This change is very easy to implement, but it will break a lot of scripts that call that functionality from Gammapy. I think there aren't many people using it and they can just adapt when going to Gammapy 0.5, I don't want to do a deprecation cycle.

@adonath @joleroi - Do you agree? Do you have a preference where to put it?

@cdeil cdeil added the cleanup label Jun 30, 2016

@cdeil cdeil added this to the 0.5 milestone Jun 30, 2016

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Jun 30, 2016

Contributor

I totally agree 👍

Contributor

joleroi commented Jun 30, 2016

I totally agree 👍

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jun 30, 2016

Member

Any preference for where to expose it?

Do you like this?

from gammapy.spectrum.powerlaw import powerlaw_flux, powerlaw_energy_flux
result = powerlaw_flux(args)

Or this

from gammapy.spectrum import PLUtils
result = PLUtils.flux(args)

Or something else?

I think once we have the spectral model classes not so many users will use these power law utils functions, this is more for implementing algorithms in Gammapy.
So maybe short and not super explicit names are OK here?

Member

cdeil commented Jun 30, 2016

Any preference for where to expose it?

Do you like this?

from gammapy.spectrum.powerlaw import powerlaw_flux, powerlaw_energy_flux
result = powerlaw_flux(args)

Or this

from gammapy.spectrum import PLUtils
result = PLUtils.flux(args)

Or something else?

I think once we have the spectral model classes not so many users will use these power law utils functions, this is more for implementing algorithms in Gammapy.
So maybe short and not super explicit names are OK here?

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Jun 30, 2016

Contributor

I prefer the first solution

Contributor

joleroi commented Jun 30, 2016

I prefer the first solution

@adonath

This comment has been minimized.

Show comment
Hide comment
@adonath

adonath Jun 30, 2016

Member

+1 for the spectrum.powerlaw sub-module and a function based API, which is also less work to implement.

Member

adonath commented Jun 30, 2016

+1 for the spectrum.powerlaw sub-module and a function based API, which is also less work to implement.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jun 30, 2016

Member

OK, I'll make this change in the coming days and take this as an opportunity to clean up function names / arguments and add a few tests for powerlaw.py.

Member

cdeil commented Jun 30, 2016

OK, I'll make this change in the coming days and take this as an opportunity to clean up function names / arguments and add a few tests for powerlaw.py.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jun 30, 2016

Member

This is what I put for now.

My suggestion would be to leave it at that, and to only continue the cleanup here in a later PR once the PowerLaw class is implement.

@joleroi - OK?

screen shot 2016-06-30 at 20 36 42

Member

cdeil commented Jun 30, 2016

This is what I put for now.

My suggestion would be to leave it at that, and to only continue the cleanup here in a later PR once the PowerLaw class is implement.

@joleroi - OK?

screen shot 2016-06-30 at 20 36 42

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Jul 1, 2016

Contributor

Perfekt!

Contributor

joleroi commented Jul 1, 2016

Perfekt!

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jul 1, 2016

Member

I'm merging this now.

(Just for the record: this will need more cleanup, better docstrings, tests added ... after the main spectral model classes are done.)

Member

cdeil commented Jul 1, 2016

I'm merging this now.

(Just for the record: this will need more cleanup, better docstrings, tests added ... after the main spectral model classes are done.)

@cdeil cdeil merged commit 32c0dac into gammapy:master Jul 1, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment