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 Fvar variability measure for light curves #798

Merged
merged 5 commits into from Dec 20, 2016
Merged

Add Fvar variability measure for light curves #798

merged 5 commits into from Dec 20, 2016

Conversation

@cnachi
Copy link
Contributor

@cnachi cnachi commented Nov 25, 2016

Please dont merge yet ; will provide a nicer example test

@cdeil cdeil self-assigned this Nov 25, 2016
@cdeil cdeil added this to the 0.6 milestone Nov 25, 2016
@cdeil cdeil added the feature label Nov 25, 2016
Copy link
Member

@cdeil cdeil left a comment

I left some inline comments.
Let me know if you have any questions or when it's ready for final review.


def compute_fvar(self):
"""Calculate the fractional excess variance (Fvar) of `LightCurve`.

This comment has been minimized.

@cdeil

cdeil Nov 25, 2016
Member

Please add a link to a paper or webpage with a description / formula for this.

Maybe also mention which columns from the lightcurve are accessed.

Also put this in the docstring:

Returns
--------
fvar, fvarerr : float
    Normalised excess variance and error on it.

This comment has been minimized.

"""
time_min = self['TIME_MIN']
time_max = self['TIME_MAX']

This comment has been minimized.

@cdeil

cdeil Nov 25, 2016
Member

Looks like you have mixed tabs and spaces?
Should all be spaces and 4 space indent.

pep8 tool will tell you about it, autopep8 tool can auto-fix it.
(or configure your editor for Python dev)

This comment has been minimized.

@cnachi

cnachi Dec 2, 2016
Author Contributor

Is this the right place to get autopep8 ?
https://github.com/tell-k/vim-autopep8

This comment has been minimized.

@cdeil

cdeil Dec 2, 2016
Member

No, this is the right autopep8:
https://pypi.python.org/pypi/autopep8

To get it, do pip install autopep8, that will fetch and install it from PyPI, the Python package index.

#ascii.write([fluxerrT],'simfluxerr.txt')#,np.sum(fluxerr),fluxerr#nptsperbin
sigxserrA = np.sqrt( 2.0/nptsperbin) * (Sigsq/phimean)**2
sigxserrB = np.sqrt(Sigsq/nptsperbin) * (2.0*fvar/phimean)
#print sigxserrA,sigxserrB

This comment has been minimized.

@cdeil

cdeil Nov 25, 2016
Member

Remove print & IPython after debugging.

This comment has been minimized.

@cnachi

cnachi Dec 2, 2016
Author Contributor

Removed comments

@@ -3,7 +3,9 @@
from astropy.units import Quantity
from astropy.tests.helper import assert_quantity_allclose
from ...utils.testing import requires_dependency
#import matplotlib.pyplot as plt

This comment has been minimized.

@cdeil

cdeil Nov 25, 2016
Member

Remove

This comment has been minimized.

@cnachi

cnachi Dec 2, 2016
Author Contributor

Removed this comment

@cdeil
Copy link
Member

@cdeil cdeil commented Dec 1, 2016

@cnachi - Do you have time to finish this up?

It's always best to try and get pull requests in quickly, otherwise it's usually hard to re-start and there's merge conflicts if others touched the file etc.
If you want, you can also come by my office and we finish it together.

@cdeil
Copy link
Member

@cdeil cdeil commented Dec 19, 2016

@cnachi - Do you have time to finish this up? Or should I just do it?

I think we discussed it and it was mostly a matter of improving the docstring and a little bit the method (e.g. unityrow = np.ones(nptsperbin) isn't needed because Numpy broadcasting will take care of that automatically).

@cnachi
Copy link
Contributor Author

@cnachi cnachi commented Dec 19, 2016

@cdeil : I committed some changes a few days ago - I forgot to alert you - I basically edited the docstring to add the formula and reference and also used the numpy broadcasting. Can you see this ?

@cdeil
Copy link
Member

@cdeil cdeil commented Dec 19, 2016

@cnachi - No, I don't see a formula in the docstring.

Each pull request has three "views" you / I / anyone can choose at the top:

  • "Conversation" at https://github.com/gammapy/gammapy/pull/798
  • "Commits" at https://github.com/gammapy/gammapy/pull/798/commits
  • "Files" at https://github.com/gammapy/gammapy/pull/798/files showing the diff

You could check the local commits using git log versus the commits on this pull request branch on github. Maybe you have local commits that you didn't push to github yet?

And yes, please after adressing some comments and pushing new commits on a PR, please leave another comment an status, i.e. whether you think the PR is ready or if you have some question, so that I know what to do.

@cnachi
Copy link
Contributor Author

@cnachi cnachi commented Dec 19, 2016

@cdeil : Ahh I think you should be able to see them now

@cdeil cdeil merged commit 3885ba6 into gammapy:master Dec 20, 2016
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@cdeil
Copy link
Member

@cdeil cdeil commented Dec 20, 2016

@cnachi - Thank you!

I've cleaned up the docstring and implementation a bit in 9d9be3b and then merged this.
Key changes are:

  • start docstring with r""", i.e. have a "raw string" to prevent Python from processing escape characters like \. That's needed when you have LaTeX in a string.
  • don't access TIME columns in the method. They aren't used to compute f_var.
@cdeil cdeil changed the title Add Fvar Function with Test to LightCurve Add Fvar variability measure for light curves Dec 20, 2016
@cnachi cnachi deleted the cnachi:fvar branch Dec 20, 2016
@cnachi
Copy link
Contributor Author

@cnachi cnachi commented Dec 20, 2016

@cdeil : Thanks for cleaning up - where does this appear in the docs of gammapy ?

@cdeil
Copy link
Member

@cdeil cdeil commented Dec 20, 2016

It should appear here: http://docs.gammapy.org/en/latest/api/gammapy.time.LightCurve.html

It doesn't at the moment because of a docs build issue: http://readthedocs.org/projects/gammapy/builds/4812909/

I'll investigate and fix in the coming days.

@cdeil
Copy link
Member

@cdeil cdeil commented Jan 8, 2017

@cnachi - just FYI: the online docs build issue has been resolved in the meantime.

Docs for the new method now are here:
http://docs.gammapy.org/en/latest/api/gammapy.time.LightCurve.html#gammapy.time.LightCurve.compute_fvar

@cnachi
Copy link
Contributor Author

@cnachi cnachi commented Jan 8, 2017

@cdeil : Great thanks !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants