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 LightCurve class #564

Merged
merged 9 commits into from Jun 10, 2016
Merged

Add LightCurve class #564

merged 9 commits into from Jun 10, 2016

Conversation

@cnachi
Copy link
Contributor

@cnachi cnachi commented Jun 9, 2016

This PR adds a LightCurve class as suggested in #563 .

Work in progress ...

cc @cdeil @helen-poon

@cdeil cdeil mentioned this pull request Jun 9, 2016
@cnachi
Copy link
Contributor Author

@cnachi cnachi commented Jun 9, 2016

@cdeil @helen-poon : preilim prototype LightCurve class ; please take a look.

@cdeil cdeil added the feature label Jun 9, 2016
@cdeil cdeil added this to the 0.5 milestone Jun 9, 2016
@cdeil cdeil self-assigned this Jun 9, 2016
@cdeil
Copy link
Member

@cdeil cdeil commented Jun 9, 2016

@cnachi - Thanks!

For me, the example doesn't work.

On Python 2 I get this error:

$ python2 examples/example_lightcurve.py 
Traceback (most recent call last):
  File "examples/example_lightcurve.py", line 5, in <module>
    from gammapy.time import LightCurve
ImportError: cannot import name LightCurve

To fix it, you will have to add the following line after the imports at the top of gammapy/time/lightcurve.py:

__all__ = ['LightCurve']

(see any other file in Gammapy for examples)

On Python 3 I get this error:

$ python3 examples/example_lightcurve.py 
Traceback (most recent call last):
  File "examples/example_lightcurve.py", line 5, in <module>
    from gammapy.time import LightCurve
  File "/Users/deil/code/gammapy/gammapy/time/__init__.py", line 8, in <module>
    from .lightcurve import *
  File "/Users/deil/code/gammapy/gammapy/time/lightcurve.py", line 42
    tstart = self.table['TSTART'].to('s')
                                        ^
TabError: inconsistent use of tabs and spaces in indentation

If you don't have your editor set up yet to do Python code formatting automatically, you can install and use the autopep8 tool to fix up your code:

pip install autopep8
autopep8 --in-place gammapy/time/lightcurve.py
git commit -am 'Run autopep8 on gammapy/time/lightcurve.py'

This will indent with four spaces and never use tabs, as is recommended by the Python style guide (which is called PEP8).

Then the example should work.

Next step is to add a test.
Add a new file called gammapy/time/tests/test_lightcurve.py
and put some test for your LightCurve class.

Maybe you could start by copy & pasting lines from here, as an example?
https://github.com/gammapy/gammapy/blob/master/gammapy/time/tests/test_simulate.py
As a test, you could copy & paste the example into a test_lightcurve function, and then compute the mean flux and add an assert_allclose on the result (here's an example: https://github.com/gammapy/gammapy/blob/master/gammapy/stats/tests/test_poisson.py#L43)

To run the test, execute the following command from the top level in the repo (where the setup.py file is located):

python setup.py test -V -t gammapy/time/tests/test_lightcurve.py

Let me know how it goes ... getting the testing part to work is probably the hardest step. If you have time tomorrow, we're again in the same room.

I will have other comments about your code and docstrings ... it's normal that the first few pull requests go through several rounds of feedback as you learn how we do things in Gammapy. But the first step is to get the example working and have a place where to put tests.

@cdeil
Copy link
Member

@cdeil cdeil commented Jun 10, 2016

Thanks!

Looks good ... merging this now.

@cdeil cdeil merged commit 5645d3e into gammapy:master Jun 10, 2016
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.