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 create fixed time interval method for light curves #1339

Merged
merged 6 commits into from Mar 30, 2018

Conversation

Projects
None yet
3 participants
@gabemery
Contributor

gabemery commented Mar 14, 2018

Added a simple function which create a light curve with a given time binning called create_fixed_time_bin_lc

Added a security in compute_flux_point which returned an error if the condition line 407 was always true for a given interval. This might happen for example if the interval is fully between two observations.
Now the function return both a boolean to say if the interval was valid and the light curve point

Added the use of the previous change in light_curve to save only the valid lc point in the final light curve object.

Emery Gabriel added some commits Mar 14, 2018

Emery Gabriel
added fixed time bin lightcurve and protection against invalid point …
…in compute_flux_point; also added a first version of a fixed significance per point lc
Emery Gabriel
@cdeil

@gabemery - Thank you for the PR!

I've left a few inline comments. Two more thoughts:

  • please add a test executing your new function once, and asserting something about the return value that shows that the function does what the docstring says.
  • your use of useinterval that introduces if statements and more complex function return values in the code seems sub-optimal. Would it be possible to simplify this, by introducing a new function which takes a list or table of time intervals, and filters out the ones with zero observations? Would that be better or worse than what you put?

Generally, I'm not sure how complex of time interval handling code we need in Gammapy. Either we develop our own little set of functionality, or if it's nontrivial, we could also copy some existing code like https://github.com/chaimleib/intervaltree into gammapy/extern and import and use that, re-exposing it in a simple way to end-users to get the common methods to compute time intervals or intersect desired time intervals with available observation intervals. Just a thought for a possible long-term solution for Gammapy.

Overall I'm not a lightcurve guy and don't have much time, so @labsaha or @jlenain - if you have time to have a look at this and give feedback here, that would be great!

"""
Create time intervals of fixed size and then a light curve using those intervals
:param time_step: float

This comment has been minimized.

@cdeil

cdeil Mar 15, 2018

Member

Please use the numpy docstring format for the parameters (see any other function / method docstring in Gammapy as example). With this, it won't render correctly in our docs build.
You can check locally via make doc && make doc-show, but you don't have to, the Sphinx build is super slow.

"""
on_evt_list = []
for obs in spec_extract.bkg_estimate:
on_evt_list.append(obs.on_events)
return on_evt_list
def create_fixed_time_bin_lc(self, time_step, spectral_model, energy_range, spectrum_extraction):
"""
Create time intervals of fixed size and then a light curve using those intervals

This comment has been minimized.

@cdeil

cdeil Mar 15, 2018

Member

Is it possible to only have a utility function to compute the time binning, and then re-use the existing methods?

We're trying to bring Gammapy towards more modular code, and one key to modularity is that one function has one responsibility. Your method has two, as demonstrated by the "and" in the docstring:

Create time intervals of fixed size and then a light curve using those intervals

We don't know how to organise the LC estimation code yet. For me, it would be reasonable to just accumulate a bit of functionality either as methods on this class, or better yet, as separate standalone functions in the same file.

@cdeil cdeil added the feature label Mar 15, 2018

@cdeil cdeil added this to the 0.8 milestone Mar 15, 2018

@cdeil cdeil changed the title from Lightcurve fixed time to Lightcurve fixed time intervals Mar 15, 2018

@gabemery

This comment has been minimized.

Contributor

gabemery commented Mar 16, 2018

Thanks for your review,
I will change the function to only create the intervals, do the test and correct the docstring.

About the useinterval usage, it is the only solution I found as counting the number of events in each intervals and filtering the empty ones didn't work (for unknown reasons). An other solution might be found but I'm not sure I will be the one to find it.
I can check the time increase in execution time due to this addition.

@cdeil

This comment has been minimized.

Member

cdeil commented Mar 16, 2018

@gabemery - Perfect.
Once there is a test case that I or others can run, we'll find a good solution for the interval filtering together (or conclude to keep as-is).

Emery Gabriel added some commits Mar 26, 2018

Emery Gabriel
Emery Gabriel
@gabemery

This comment has been minimized.

Contributor

gabemery commented Mar 29, 2018

The change to only give the interval with the new function has been done as well as the test and a re-writing of the docstring. if all of this seems ok my proposition is to merge as it is for the "useinterval" in order to have a fix. Optimization for the execution time can then be done in another pull request

Partially cleaned. Use intervaltree to be discussed later.

@jlenain

Hi @gabemery ,
The changes look satisfactory to me. The use of https://github.com/chaimleib/intervaltree could indeed be discussed in a future pull request.

@jlenain jlenain merged commit c391a72 into gammapy:master Mar 30, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@cdeil

This comment has been minimized.

Member

cdeil commented Apr 4, 2018

@gabemery @jlenain - Thanks!

Small follow-up commit: 241944a
Sphinx is super picky about docstring formatting, and in tests we have to add @requires_dependency('scipy') for optional dependencies like scipy, so that those tests are skipped in continuous integration. (currently we only have Numpy and Astropy as required dependencies, and others like Scipy or Healpy or Matplotlib ... are optional).

@cdeil cdeil changed the title from Lightcurve fixed time intervals to Add create fixed time interval method for light curves Apr 11, 2018

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