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 exptest time variability test #565

Merged
merged 16 commits into from Jul 7, 2016

Conversation

Projects
None yet
2 participants
@helen-poon
Contributor

helen-poon commented Jun 9, 2016

a function to calculate the Mr value for exptest

@cdeil cdeil added the feature label Jun 9, 2016

@cdeil cdeil added this to the PyGamma1 milestone Jun 9, 2016

@cdeil cdeil self-assigned this Jun 9, 2016

@cdeil

This comment has been minimized.

Member

cdeil commented Jun 9, 2016

Thanks for the pull request.

Please add / change the following things in this pull request:

  • Add a file examples/example_variability.py which shows a small usage example for this functionality.
  • Add a file gammapy/time/tests/test_variability.py which contains a test for this.
  • Add this at the top of the file:
# Licensed under a 3-clause BSD style license - see LICENSE.rst
from __future__ import absolute_import, division, print_function, unicode_literals

__all__ = [
    'exptest_for_run2',
]
  • Add the line from .variability import * to the gammapy/time/init.py file.
  • Fix the docstring to comply with the Numpy docstring standard.

@helen-poon helen-poon force-pushed the helen-poon:Mrvalue branch from f7bdb50 to 67b6c42 Jun 10, 2016

@cdeil cdeil modified the milestones: 0.5, PyGamma1 Jun 10, 2016

@cdeil cdeil changed the title from add Mrvalue for gamma/time/variability to Add exptest time variability test Jun 10, 2016

'exptest_for_run',
]
def exptest_for_run(time_delta=[]):

This comment has been minimized.

@cdeil

cdeil Jun 10, 2016

Member

Remove the default argument of an empty list here, i.e. write:

def exptest_for_run(time_delta):

(because it doesn't make sense to call this function without passing any inputs.

def exptest_for_run(time_delta=[]):
"""Compute Mr value, the level of variability for a certain period of time.
Longer description: A single Mr value can be calculated, which shows the level of variability

This comment has been minimized.

@cdeil

cdeil Jun 10, 2016

Member

Remove the text "Longer description:" here.
I.e. keep the description, but don't preface it with "Longer description:"

Parameters
----------
run : list of int

This comment has been minimized.

@cdeil

cdeil Jun 10, 2016

Member

Update description of input parameters.
Now there's only one input called time_delta, not the parameters you list here.

@cdeil

This comment has been minimized.

Member

cdeil commented Jun 10, 2016

As discussed in person today, the test should be replaced with this:
https://gist.github.com/cdeil/40b4914be44a3575416b7c7cd8c8311d

I.e. it must be in a function where the name starts with test_ there must be an assertion at the end and you shouldn't run a long simulation.
You can use this command locally to run and debug the test:

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

Let me know when I should have a look again or if you have any examples!

@helen-poon helen-poon force-pushed the helen-poon:Mrvalue branch from 8084798 to 635835f Jun 22, 2016

@helen-poon

This comment has been minimized.

Contributor

helen-poon commented Jun 22, 2016

I corrected the files according to all your comments.

@@ -0,0 +1,21 @@
import numpy as np

This comment has been minimized.

@cdeil

cdeil Jun 22, 2016

Member

Please add these two boilerplate lines at the top of gammapy/time/tests/test_exptest.py:

# Licensed under a 3-clause BSD style license - see LICENSE.rst
from __future__ import absolute_import, division, print_function, unicode_literals

We have those in every library and test file in Gammapy.

# setup
rate = Quantity(100, 's^-1')
time_delta = make_random_times_poisson_process(1000, rate=rate, random_state=0)
print('time_delta = {}'.format(time_delta))

This comment has been minimized.

@cdeil

cdeil Jun 22, 2016

Member

Remove print statements (there's a second one below) from the test.
Also the setup, execution and checks on results comments can be removed.
This was just for explanation ... it's the structure of all tests, but we don't put comments explaining where each section starts, just usually a blank line between those sections.

import numpy as np
__all__ = [
'exptest_for_run',

This comment has been minimized.

@cdeil

cdeil Jun 22, 2016

Member

Would it be OK for you to just rename this function from exptest_for_run to exptest?
The term "run" probably won't be used in CTA ... if you want to keep it use exptest_for_observation.
But I'd prefer to just have the simpler name exptest for this function.

@cdeil

This comment has been minimized.

Member

cdeil commented Jun 22, 2016

@helen-poon - I left some inline comments.

If you look at #565 and scroll to the bottom, you'll see the message "This branch has conflicts that must be resolved. Use the command line to resolve conflicts before continuing.".

The merge conflict is probably in gammapy/time/__init__.py, because since you started this branch the same line was edited and this was added:

from .lightcurve import *

(see https://github.com/gammapy/gammapy/blob/master/gammapy/time/__init__.py#L8)

So what you should do here to resolve the merge conflict is to rebase on trunk as described here:
http://docs.astropy.org/en/latest/development/workflow/additional_git_topics.html#rebasing-on-trunk

If you want to try by yourself, go ahead.
But I'm also around tomorrow and we could just do it together.

I'll look at the implementation of the exptest function more closely tomorrow ... maybe there's a way to avoid the Python for loop and make it faster.

M_value=0
Mr=0
sum_time_all = np.sum([sum_time])
if len(time_delta)!=0:

This comment has been minimized.

@cdeil

cdeil Jun 22, 2016

Member

I would suggest to remove the check if len(time_delta)!=0 here.

Just assume callers don't pass empty data.
And if they do, your function raises a ZeroDivisionError ... garbage in, garbage out.

Or wait ... for this function, is returning Mr=0 for empty data helpful?
If so, then a better way is to put:

if len(time_delta) == 0:
    return 0

...

at the very top of the function, instead of having this special case handled further down, and the "normal" calculation indented.

OK?

if normalized_time_delta[i] < 1:
sum_time.append(1 - normalized_time_delta[i] / 1.0)
mean_normalized_time=np.mean(normalized_time_delta)
M_value=0

This comment has been minimized.

@cdeil

cdeil Jun 22, 2016

Member

This line M_value=0 can be removed, no?

"""
mean_time = np.mean(time_delta)
normalized_time_delta = time_delta / mean_time
sum_time = []

This comment has been minimized.

@cdeil

cdeil Jun 22, 2016

Member

I haven't looked at your code in detail, but can't you avoid the for loop and do the same computation using just Numpy array expressions roughly like this?

mask = normalized_time_delta < 1
sum_time = 1 - normalized_time_delta[mask] / 1

This is using "fancy indexing using boolean arrays":
http://www.scipy-lectures.org/intro/numpy/array_object.html#using-boolean-masks

There's other Numpy tricks that will usually let you avoid Python for loops and get much better performance.
Let me know if this doesn't work, then I'll have a closer look.

plt.grid(True)
plt.show()
def readinfile():

This comment has been minimized.

@cdeil

cdeil Jun 22, 2016

Member

This readinfile function is now a single line.
Remove it and just put the single line where you call it.

@cdeil

This comment has been minimized.

Member

cdeil commented Jun 22, 2016

In examples/example_exptest.py, you're not importing the function from gammapy/time/exptest.py.
So I don't think your example code works!?

And can you please clean up the code formatting in the examples/example_exptest.py?
https://gist.github.com/cdeil/eb4fa4c8a86c032c64a77b462973ae35
A good Python code editor can do this for you, and there's also the autopep8 command line tool to help.
Again, if you don't know how to do this or it doesn't work, the easiest would be that we do it together tomorrow.
Let me know ...

@helen-poon helen-poon force-pushed the helen-poon:Mrvalue branch from 635835f to c5ced9a Jun 23, 2016

@helen-poon

This comment has been minimized.

Contributor

helen-poon commented Jul 6, 2016

I modified the code of example_exptest.py and cleaned it using pycharm, not sure whether it is clean enough.

@helen-poon helen-poon force-pushed the helen-poon:Mrvalue branch from bc040f9 to 9d41c83 Jul 6, 2016

@cdeil cdeil merged commit 9d41c83 into gammapy:master Jul 7, 2016

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@cdeil

This comment has been minimized.

Member

cdeil commented Jul 7, 2016

@helen-poon - Thank you!

I've merges this PR just now, including the improvements we did together for the example: 55c7136

I don't have time to finish rewriting exptest_multi in examples/example_exptest.py to use Numpy array expressions ... maybe we can do that together some time.

I've added you to the Gammapy contributors list here:
https://gammapy.readthedocs.io/en/latest/about.html#contributors

And this PR is listed in the changelog here:
https://gammapy.readthedocs.io/en/latest/changelog.html#gammapy-0p5-release

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