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 ObservationTableSummary class #531

Merged
merged 9 commits into from May 19, 2016

Conversation

Projects
None yet
3 participants
@jjlk
Contributor

jjlk commented May 18, 2016

Hi,
This is my first pull request as we discussed with @cdeil. Adding a new class to summarize and plots the content of an ObservationTable. The test part will come later.

@cdeil cdeil added the feature label May 18, 2016

@cdeil cdeil added this to the 0.5 milestone May 18, 2016

@cdeil cdeil self-assigned this May 18, 2016

Class allowing to summarize informations conatained in
Observation index table (`~gammapy.data.ObservationTable`)
Parameters:

This comment has been minimized.

@cdeil

cdeil May 18, 2016

Member

Try running this locally:

python setup.py build_sphinx
open docs/_build/html/index.html

and look at the API docs for the ObservationTableSummary class.

You will find that it's not formatted correctly.
You have to remove the colon here and add an extra space below, i.e. make it look like this:

    Parameters
    ----------
    obs_table :  `~gammapy.data.ObservationTable`
        Observation table
    target_pos : `~astropy.coordinates.SkyCoord`
        Target position

This comment has been minimized.

@cdeil

cdeil May 18, 2016

Member

This comment concerning the class docstring Sphinx formatting isn't addressed yet.

self.obs_table = obs_table
self.target_pos = target_pos
def get_offset(self):

This comment has been minimized.

@cdeil

cdeil May 18, 2016

Member

Maybe make this private (by adding an underscore), i.e. not part of the public API?
And this is nicer as a property:

@property
def _offset(self):
    pnt_pos = SkyCoord(self.obs_table['RA_PNT'], self.obs_table['DEC_PNT'], unit='deg')
    return pnt_pos.separation(self.target_pos)
def plot_offset_distribution(self, ax=None, bins=10, range=(0,3.)):
"""
Construct the zenith distribution of the observations

This comment has been minimized.

@cdeil

cdeil May 18, 2016

Member

Copy & paste error: "zenith" -> "offset"

return pnt_coord.separation(self.target_pos)
def plot_zenith_distribution(self, ax=None, bins=100, range=(0,100.)):

This comment has been minimized.

@cdeil

cdeil May 18, 2016

Member

I would prefer if you pass in the binning spec like this:

def plot_something(bins=None):
    zen = self.obs_table['ZEN_PNT']
    if bins is None:
        bins = np.linspace(0, zen.max() + 5, 10)

This has advantages:

  1. The caller can pass in an arbitrary binning.
  2. There's just one arg, not two
  3. You have more flexibility to compute the default via code

It's hard to set good defaults for plots.
Here probably 0 ... 90 would be acceptable as well and it's pretty easy.
In other plots we have hard-coded values that are appropriate for HESS, i.e. offset 0 ... 2.5.

I'm not sure how to do this better ... I do think it's important to try to put defaults so that it's easy to use for common cases.

@cdeil

This comment has been minimized.

Member

cdeil commented May 18, 2016

@jjlk - Thanks!

It mostly looks good ... I've left a bunch of minor inline comments.
I would prefer if you add tests in this PR instead of a follow-up PR.
OK?

@jjlk

This comment has been minimized.

Contributor

jjlk commented May 18, 2016

Yes, I'm working on it! I will add the tests in this request.

@cdeil

This comment has been minimized.

Member

cdeil commented May 18, 2016

I will add the tests in this request.

Thanks!

Note that you have to leave a comment here if you want me to have another look.
Github doesn't send notifications if someone pushes extra commits to a pull request (PR).

Fix doc format, add property to offset function, raise exception for …
…to be implemented function and add default binning for plots
Returns
-------
vector : `~numpy.array`

This comment has been minimized.

@joleroi

joleroi May 18, 2016

Contributor

You can directly return an ~astropy.Angle here.
At some point we decided to use Quantity (and thus also Angle) in gammapy.

This comment has been minimized.

@jjlk

jjlk May 18, 2016

Contributor

Oké thanks !

self.target_pos = target_pos
@property
def _offset(self):

This comment has been minimized.

@joleroi

joleroi May 18, 2016

Contributor

Did you hide this property intentionally? I have never seen a hidden property before, but that does not necessarily mean anything :)

This comment has been minimized.

@jjlk

jjlk May 18, 2016

Contributor

It was suggested by @cdeil, and I'm not a python expert =)

This comment has been minimized.

@cdeil

cdeil May 18, 2016

Member

I'm 50:50 on whether this should be public or private.
@jjlk or @joleroi - If you think it's useful to have for users of this class, make it public (i.e. remove the underscore).

@jjlk

This comment has been minimized.

Contributor

jjlk commented May 18, 2016

Hi, I pushed the test functions and I made the offset function public. Tell me if it's ok for you ++

Summary table
"""
datas_tore = DataStore.from_dir('$GAMMAPY_EXTRA/datasets/hess-crab4-hd-hap-prod2/')

This comment has been minimized.

@cdeil

cdeil May 18, 2016

Member

There's this error: https://travis-ci.org/gammapy/gammapy/jobs/131172223#L924

You should see that locally as well when you run this test, no?

python setup.py test -V -t gammapy/data/tests/test_observation_summary.py
from ...utils.testing import data_manager, requires_data, requires_dependency
from ...datasets import gammapy_extra

This comment has been minimized.

@cdeil

cdeil May 18, 2016

Member

Remove this unused import:

from ...datasets import gammapy_extra
from gammapy.data import DataStore
from gammapy.data import ObservationTable
from gammapy.data import ObservationTableSummary

This comment has been minimized.

@cdeil

cdeil May 18, 2016

Member

In Gammapy, we use explicit relative imports everywhere.
So to be consistent, please change this to

from ...data import DataStore, ObservationTable, ObservationTableSummary
summary = init_summary()
plt.figure()

This comment has been minimized.

@cdeil

cdeil May 18, 2016

Member

Remove the following lines from the plot test:

import matplotlib.pyplot as plt
plt.figure()
plt.savefig('output/plot_zenith_distribution.pdf')

Add the following decorator before this test function:

requires_dependency('matplotlib')

It would be better to split the test for the zenith and the offset plot into two different test functions (each just two lines long).

@property
def offset(self):
"""

This comment has been minimized.

@cdeil

cdeil May 18, 2016

Member

For this I would suggest a shorter docstring as described here:
http://docs.gammapy.org/en/latest/development/howto.html#functions-or-class-methods-that-return-a-single-object

@property
def offset(self):
    """Offset of observations relative to the target position (`astropy.coordinates.Angle`)."""
offset = pnt_pos.separation(self.target_pos)
return offset.degree

This comment has been minimized.

@cdeil

cdeil May 18, 2016

Member

Calling .degree on an Angle object drops the units

>>> from astropy.coordinates import Angle
>>> Angle(10, 'deg').degree
10.0

In this case I think you can just remove the .degree attribute access, offset is already an Angle object in degree.

For the future ... if you want to change units for an angle, but still have an Angle object, use the .to method:

>>> Angle(1000, 'arcsec').to('deg')
<Angle 0.2777777777777778 deg>
Axis
bins : integer
number of bins, optional
range : `range`

This comment has been minimized.

@cdeil

cdeil May 18, 2016

Member

The docstring now no longer matches the function signature.
Remove the range option and update the description of bins.

Returns
--------
ax : `~matplolib.axes`

This comment has been minimized.

@cdeil

cdeil May 18, 2016

Member

The docstring for ax should be this everywhere:

    ax : `~matplotlib.axes.Axes` or None, optional.
        The `~matplotlib.axes.Axes` object to be drawn on.
        If None, uses the current `~matplotlib.axes.Axes`.

I see that this is incorrect in the example I showed you earlier ... this should be fixed everywhere in a separate cleanup pull request.

return ax
def __str__(self):

This comment has been minimized.

@cdeil

cdeil May 18, 2016

Member

Please add a test for __str__.
It should look something like this:

def test_str(self):

The main point is to call str(obs_summary) somewhere to execute this.
Otherwise what will happen for such code that isn't covered is that someone changes the class and this method breaks.
It would be nice to have an assert, it could just be this:

text = str(obs_table)
assert 'Summary report' in text

although if you want to assert on more or other parts of the text, that's fine as well of course.

"""Observation table summary.
Class allowing to summarize informations conatained in
Observation index table (`~gammapy.data.ObservationTable`)

This comment has been minimized.

@cdeil

cdeil May 18, 2016

Member

Typo: 'conatained' -> 'contained'

@cdeil

This comment has been minimized.

Member

cdeil commented May 18, 2016

@jjlk - I've done another round of inline comments. A bunch of little stuff, nothing major.

@jjlk

This comment has been minimized.

Contributor

jjlk commented May 19, 2016

Hi @cdeil, sorry for the tests, in fact they weren't done on my machine. I fixed the problems and I took into account your comments. Is it ok now ?

Class allowing to summarize informations contained in
Observation index table (`~gammapy.data.ObservationTable`)
Parameters:

This comment has been minimized.

@cdeil

cdeil May 19, 2016

Member

Docstring for ObservationTableSummary still not ok.
Check locally with

python setup.py build_sphinx -l
open docs/_build/html/index.html
Returns
--------
ax : `~matplolib.axes`

This comment has been minimized.

@cdeil

cdeil May 19, 2016

Member

~matplotlib.axes.Axes

Returns
-------
ax : `~matplolib.axes`

This comment has been minimized.

@cdeil

cdeil May 19, 2016

Member

~matplotlib.axes.Axes

@requires_data('gammapy-extra')
def test_str():
"""Test if str is well computed"""

This comment has been minimized.

@cdeil

cdeil May 19, 2016

Member

I would suggest you remove all these docstrings on your test functions.
They don't add extra info wrt the code.

data_store = DataStore.from_dir('$GAMMAPY_EXTRA/datasets/hess-crab4-hd-hap-prod2/')
observations = data_store.obs_table
summary = ObservationTableSummary(observations,SkyCoord.from_name('crab'))

This comment has been minimized.

@cdeil

cdeil May 19, 2016

Member

Don't use SkyCoord.from_name here. This does a SESAME query, which is a potential cause for trouble when running tests.
Instead hard-code the Crab position:

target_pos = SkyCoord(83.633083, 22.0145, unit='deg')
from ...utils.testing import data_manager, requires_data, requires_dependency
def init_summary():

This comment has been minimized.

@cdeil

cdeil May 19, 2016

Member

For this I would recommend you put a pytest.fixture. Such fixtures look very weird at first, but they are very powerful and a very good way to create test objects and "inject" them into test functions.

In this case I think this should do it (I didn't test):

@requires_data('gammapy-extra')
def summary():
    data_store = DataStore.from_dir('$GAMMAPY_EXTRA/datasets/hess-crab4-hd-hap-prod2/')
    target_pos = SkyCoord(83.633083, 22.0145, unit='deg')
    return ObservationTableSummary(data_store.obs_table, target_pos)

And then you use the fixture like this, i.e. inject it as an input parameter into the test function with the same name as the fixture function above:

def test_str(summary):
    text = str(summary)
    assert 'Observation summary' in text
@cdeil

This comment has been minimized.

Member

cdeil commented May 19, 2016

I've done a final review. My main comment is that you should use a pytest fixture.
This is a bit magical Python code ... it works because pytest is running the tests, not normal Python.

Once that change is made, this is ready to be merged.

@jjlk

This comment has been minimized.

Contributor

jjlk commented May 19, 2016

Thanks @cdeil, job done !

return ObservationTableSummary(data_store.obs_table, target_pos)
@requires_data('gammapy-extra')
def test_str():

This comment has been minimized.

@cdeil

cdeil May 19, 2016

Member

You should inject the fixture as an argument into the test function like this:

@requires_data('gammapy-extra')
def test_str(summary):
    text = str(summary)
    assert 'Observation summary' in text
text = str(summary())
assert 'Observation summary' in text
@requires_data('gammapy-extra')

This comment has been minimized.

@cdeil

cdeil May 19, 2016

Member

I think if you declare @requires_data('gammapy-extra') on the fixture, you don't need to declare it on the test functions that use the fixture. So please remove this on the test functions.

@requires_data('gammapy-extra')
def test_offset():
offset = summary().offset
assert ((offset.degree.mean() - 1.0) <1.e-3 and (offset.degree.std() - 0.5) <1.e-3)

This comment has been minimized.

@cdeil

cdeil May 19, 2016

Member

This is a bad way to write assertions, because if they fail, you don't get a good error message about what the actual values were. Try it out by putting a wrong value and read the error message!

Please use assert_quantity_allclose:
http://docs.gammapy.org/en/latest/development/howto.html#assert-convention

ax : `~matplotlib.axes.Axes` or None, optional.
The `~matplotlib.axes.Axes` object to be drawn on.
If None, uses the current `~matplotlib.axes.Axes`.
bins : integer

This comment has been minimized.

@cdeil

cdeil May 19, 2016

Member

I would suggest to put the following more accurate description of the bins option here and in the other plot method:

"""
bins : integer or array_like, optional
    Binning specification, passed to `matplotlib.pyplot.hist`.
    By default, 30 bins from 0 deg to max zenith + 5 deg is used.

This will create a link from the Gammapy docs to the matplotlib docs here:
http://matplotlib.org/api/pyplot_api.html#matplotlib.pyplot.hist

@jjlk

This comment has been minimized.

Contributor

jjlk commented May 19, 2016

@cdeil: ok done.

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

This comment has been minimized.

@cdeil

cdeil May 19, 2016

Member

We use the bundled pytest version in Astropy everywhere:

from astropy.tests.helper import pytest
def test_offset(summary):
offset = summary.offset
assert_allclose(offset.degree.mean(),1.,rtol=1.e-2,atol=0.)

This comment has been minimized.

@cdeil
If None, uses the current `~matplotlib.axes.Axes`.
bins : integer or array_like, optional
Binning specification, passed to `matplotlib.pyplot.hist`.
By default, 30 bins from 0 deg to max zenith + 5 deg is used.

This comment has been minimized.

@cdeil

cdeil May 19, 2016

Member

Copy & paste error ... describe offset binning here.

@cdeil

This comment has been minimized.

Member

cdeil commented May 19, 2016

Very close now ... :-)

@cdeil

This comment has been minimized.

Member

cdeil commented May 19, 2016

About the @requires_data('gammapy-extra') decorator I was mistaken.
This now fails in continuous integration:
https://ci.appveyor.com/project/cdeil/gammapy/build/1.0.656/job/4hwmk6j8u9v0a832#L1095
Apparently it has to be added to every test function that accesses data from gammapy-extra, not just the fixture. I'll try to fix this at some point, but for now, please add the decorator back to all your tests.
Sorry for misleading you there ...

@jjlk

This comment has been minimized.

Contributor

jjlk commented May 19, 2016

@cdeil: I guess that this is the one !

@cdeil

This comment has been minimized.

Member

cdeil commented May 19, 2016

Looks good.
Merging this now.

Congratulations on your patience with the code review!
😄

@cdeil cdeil merged commit 398ebe1 into gammapy:master May 19, 2016

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 May 19, 2016

Just to wrap this up:

I use https://www.jetbrains.com/pycharm/ as Python IDE and I can recommend it. It has a free edition. It points out errors and completions as you type, has nice code navigation features and does such cleanup by pressing a shortcut.

@jjlk

This comment has been minimized.

Contributor

jjlk commented May 20, 2016

Hi @cdeil, thanks for correcting the typos and for the tips.

@joleroi

This comment has been minimized.

Contributor

joleroi commented May 20, 2016

@jjlk

This comment has been minimized.

Contributor

jjlk commented May 20, 2016

@joleroi
I'm emacs-biased =). So I might have a try to this one https://github.com/jorgenschaefer/elpy

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