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 PhaseCurve class for periodic systems #1300

Merged
merged 5 commits into from Feb 14, 2018

Conversation

Projects
None yet
2 participants
@labsaha
Contributor

labsaha commented Feb 9, 2018

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 cdeil self-assigned this Feb 9, 2018

@cdeil cdeil added the feature label Feb 9, 2018

@cdeil cdeil added this to the 0.8 milestone Feb 9, 2018

@cdeil

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

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

This comment has been minimized.

@cdeil

cdeil Feb 9, 2018

Member

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

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

This comment has been minimized.

@cdeil

cdeil Feb 9, 2018

Member

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.

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

This comment has been minimized.

@cdeil

cdeil Feb 14, 2018

Member

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.

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

This comment has been minimized.

@cdeil

cdeil Feb 14, 2018

Member

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

This comment has been minimized.

@labsaha

labsaha Feb 14, 2018

Contributor

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

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

This comment has been minimized.

@cdeil

cdeil Feb 14, 2018

Member

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

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

This comment has been minimized.

@cdeil

cdeil Feb 14, 2018

Member

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

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

This comment has been minimized.

@cdeil

cdeil Feb 14, 2018

Member

remove print statement

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

This comment has been minimized.

@cdeil

cdeil Feb 14, 2018

Member

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

labsaha and others added some commits Feb 14, 2018

@cdeil

This comment has been minimized.

Member

cdeil commented Feb 14, 2018

@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:

screen shot 2018-02-14 at 15 51 34

@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

This comment has been minimized.

Contributor

labsaha commented Feb 14, 2018

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

@cdeil

This comment has been minimized.

Member

cdeil commented Feb 14, 2018

@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

cdeil approved these changes Feb 14, 2018

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

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

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
@cdeil

This comment has been minimized.

Member

cdeil commented Feb 14, 2018

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

@labsaha

This comment has been minimized.

Contributor

labsaha commented Feb 14, 2018

@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

This comment has been minimized.

Contributor

labsaha commented Feb 15, 2018

@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

This comment has been minimized.

Member

cdeil commented Feb 15, 2018

@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.

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

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