Add PhaseCurve class for periodic systems #1300

labsaha · 2018-02-09T13:59:35Z

This PR adds PhaseCurve class for calculating the phase of a periodic system at a given time. It also calculates the normalization factor at that phase of the system by ring interpolation using a given table of phase vs normalization factor.

cdeil

@labsaha - I left two inline comments. Let me know if you have any questions.

cdeil · 2018-02-09T15:58:30Z

gammapy/time/models.py

+log = logging.getLogger(__name__)
+
+
+class PhaseCurve(object):


@labsaha - all of the code in the class (including the docstring) should be indented.

cdeil · 2018-02-09T15:59:30Z

gammapy/time/tests/test_models.py

+
+
+@pytest.fixture(scope='session')
+def phase():


Please call this phase_curve since it is a PhaseCurve object.

cdeil

@labsaha - I left a few inline comments, mostly for clean-up. Please do that and push the changes as another commit.

I'll then have a closer look at the API and implementation.

cdeil · 2018-02-14T11:43:58Z

gammapy/time/models.py

+    Φ(t)=Φ0+f0(t−t0)+(1/2)f1(t−t0)^2+(1/6)f2(t−t0)^3
+
+
+    Returns


The class docstring should not have a returns section.
It has a Parameters section that documents the parameters of __init__.
__init__ always returns an instance, doesn't contain a return statement and doesn't need a documentation of the return value.
Presumably here you're documenting the return value of one of the methods. That should be in the docstring of that method.

cdeil · 2018-02-14T11:44:28Z

gammapy/time/models.py

+    normalization factor for the phase using circular interpolation. 
+    The required data for interpolation is given as an astropy table
+
+    # TODO: 1. Checking table values if the phases are in increasing order


Can you resolve the TODOs? Or explain what the problem is? Is it needed?

Actually, we don't need these TODOs. This was for some checks. Now I feel that it is not correct to put these checks. Hence, I remove TODOs from the docstring

cdeil · 2018-02-14T11:45:20Z

gammapy/time/models.py

+    'PhaseCurve',
+]
+
+log = logging.getLogger(__name__)


There's no need to emit log messages from this file. Please remove this getLogger call and the import logging above.

cdeil · 2018-02-14T11:45:38Z

gammapy/time/models.py

@@ -0,0 +1,109 @@
+from __future__ import absolute_import, division, print_function, unicode_literals


At the very top of each file in Gammapy we have the license line. Please add it.

cdeil · 2018-02-14T11:46:54Z

gammapy/time/tests/test_models.py

+@pytest.fixture(scope='session')
+def phase_curve():
+    filename = make_path('$GAMMAPY_EXTRA/test_datasets/phasecurve_LSI_DC.fits')
+    print(filename)


remove print statement

cdeil · 2018-02-14T11:47:13Z

gammapy/time/tests/test_models.py

+import numpy as np
+import pytest
+from numpy.testing import assert_allclose
+from gammapy.time.models import PhaseCurve


use relative imports for gammapy, like from all other files.

cdeil · 2018-02-14T14:55:46Z

@labsaha - I've done some cleanup in e0946d3 .

I used python setup.py build_docs locally to check the formatting of the docstring. It now looks like this:

@labsaha - I would suggest to merge this PR now, and then to re-factor later once we have some "callers" for this code, i.e. start to work with these models and get some experience. OK?

labsaha · 2018-02-14T14:59:53Z

@cdeil The modification you did looks perfect! So we can merge this PR.

cdeil · 2018-02-14T15:24:52Z

@labsaha - I've done some more edits in fff5ca5, trying to make the evaluate calls more clear and modular / testable.

Overall I'm not convinced yet by what we have here, the main question is if / where we should use astropy.time.Time objects, or otherwise if we only use floats if they should be seconds (common for pulsars) or days (common for binaries), or if we use time-like quantities that have a unit attached.

This will need further thought and discussion by binary / pulsar people.

I'm merging this PR now, and then we will change / refactor as we get experience with time models.

cdeil · 2018-02-14T15:26:07Z

@labsaha - Congratulations on getting your first PR merged into Gammapy!
Anticipating this I already added you to http://gammapy.org/team.html yesterday.
:-)

labsaha · 2018-02-14T15:33:21Z

@cdeil Thanks a lot for all your help to reach this level!!!!!!! :-)

I have also this time issue in my mind. To make it clear about the model used in 1DC data, I already contacted Masha and Roberta to discuss this issue. Once we have a more clear idea on this, we can modify the code.

labsaha · 2018-02-15T23:02:13Z

@cdeil Regarding time units - after going through the XML files of for the binaries of 1 DC data challenge and some verification with the orbital phases of some binaries, I understood that to calculate the phase for a pulsar or a binary the time should be only given as MJD. Of course, it is entirely due to the reference phase given in the XML file in units of MJD. However, for real data we should keep the possibility to use time as astropy.time.Timeobject such that user can decide how to provide the time to calculate the phase.

cdeil · 2018-02-15T23:12:46Z

@labsaha - Basically at this moment you're the only dev / user of this code, so it's up to you where to use Time objects. If you want to make changes to code or docs or tests that establish behaviour, you can any time send a new pull request, or you can also group changes to this class in your next PR where you add first code to compute phase curves, or at least use times and phase curves in some real application.

Adding PhaseCurve class for periodic system

287aadf

cdeil self-assigned this Feb 9, 2018

cdeil added the feature label Feb 9, 2018

cdeil added this to the 0.8 milestone Feb 9, 2018

cdeil mentioned this pull request Feb 9, 2018

Adding a class to calculate phase folded light curve #1234

Closed

cdeil suggested changes Feb 9, 2018

View reviewed changes

Updating PhaseCurve class

061dd8c

cdeil suggested changes Feb 14, 2018

View reviewed changes

labsaha and others added 2 commits February 14, 2018 14:31

Updating the class

dece764

Improve PhaseCurve in gammapy.time.models

e0946d3

Change PhaseCurve evaluate to make it a bit more modular

fff5ca5

cdeil approved these changes Feb 14, 2018

View reviewed changes

cdeil modified the milestones: 0.8, 0.7 Feb 14, 2018

cdeil merged commit 14a4c37 into gammapy:master Feb 14, 2018

cdeil changed the title ~~Adding PhaseCurve class for periodic system~~ Add PhaseCurve class for periodic systems Feb 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PhaseCurve class for periodic systems #1300

Add PhaseCurve class for periodic systems #1300

labsaha commented Feb 9, 2018

cdeil left a comment

cdeil Feb 9, 2018

cdeil Feb 9, 2018

cdeil left a comment

cdeil Feb 14, 2018

cdeil Feb 14, 2018

labsaha Feb 14, 2018

cdeil Feb 14, 2018

cdeil Feb 14, 2018

cdeil Feb 14, 2018

cdeil Feb 14, 2018

cdeil commented Feb 14, 2018

labsaha commented Feb 14, 2018

cdeil commented Feb 14, 2018

cdeil commented Feb 14, 2018

labsaha commented Feb 14, 2018 •

edited

Loading

labsaha commented Feb 15, 2018 •

edited

Loading

cdeil commented Feb 15, 2018

		Φ(t)=Φ0+f0(t−t0)+(1/2)f1(t−t0)^2+(1/6)f2(t−t0)^3


		Returns

		@@ -0,0 +1,109 @@
		from __future__ import absolute_import, division, print_function, unicode_literals



		@pytest.fixture(scope='session')
		def phase():

Add PhaseCurve class for periodic systems #1300

Add PhaseCurve class for periodic systems #1300

Conversation

labsaha commented Feb 9, 2018

cdeil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdeil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdeil commented Feb 14, 2018

labsaha commented Feb 14, 2018

cdeil commented Feb 14, 2018

cdeil commented Feb 14, 2018

labsaha commented Feb 14, 2018 • edited Loading

labsaha commented Feb 15, 2018 • edited Loading

cdeil commented Feb 15, 2018

labsaha commented Feb 14, 2018 •

edited

Loading

labsaha commented Feb 15, 2018 •

edited

Loading