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

Document observation tables and improve gammapy.obs #278

Merged
merged 15 commits into from Jun 12, 2015

Conversation

Projects
None yet
3 participants
@mapazarr
Member

mapazarr commented Jun 2, 2015

The method still needs some work and clean-up. I am committing a preliminary version in order to trigger a discussion in the hangout tomorrow. Please don't look/comment on the minor details yet, but rather the API and the way of how to fill the obs table. I think maybe some other methods should be defined, like FillOneObservation and separate the building of a obs list from the faking of a list.
The method has a working test.

# random points between the start of 2010 and the end of 2014
#TODO: should this represent the time at the beginning of the run?
# this has consequences for the ra/dec -> alt/az conversion
datestart = Time('2010-01-01T00:00:00', format='fits', scale='utc')

This comment has been minimized.

@cdeil

cdeil Jun 3, 2015

Member

The test fails on travis-ci: https://travis-ci.org/gammapy/gammapy/jobs/65169219#L1197
You should see the same failure locally, no?

This comment has been minimized.

@mapazarr

mapazarr Jun 3, 2015

Member

@cdeil Sorry, the travis-ci took a bit longer than expected and then I forgot to check it.
The error
ValueError: Format 'fits' is not one of the allowed formats ['astropy_time', 'byear', 'byear_str', 'cxcsec', 'datetime', 'decimalyear', 'gps', 'iso', 'isot', 'jd', 'jyear', 'jyear_str', 'mjd', 'plot_date', 'unix', 'yday']
is due to a missing format (fits format) in ~astropy.Time:
http://astropy.readthedocs.org/en/latest/time/
Updating to the most recent version of astropy-dev solves the problem.
@cdeil : what is the rule about dependencies for astropy? Should we go with the latest version, or should we "freeze" a certain (stable?) version from time to time?

This comment has been minimized.

@cdeil

cdeil Jun 3, 2015

Member

@mapazarr On travis-ci we currently test with the latest dev and stable version of Astropy. The stable version is currently 1.0.2 (see https://travis-ci.org/gammapy/gammapy/jobs/65301895#L436). So you have to make this example work with Astropy 1.0.2.

In this case you can just use format=iso instead of format=fits, no?

This comment has been minimized.

@mapazarr

mapazarr Jun 3, 2015

Member

OK, I see. Yes I'll take iso for the moment.

@@ -64,3 +70,90 @@ def sigma_energy_theta(energy, theta, sigma):
zenith=zenith_hi)
return psf
def generate_observation_table(observatory, n_obs):

This comment has been minimized.

@cdeil

cdeil Jun 3, 2015

Member

I would prefer make_test_observation_table.
For consistency I prefer make over generate (see here), and I'd put in the test because this is not for science analysis, but mainly for testing (and maybe for an example, because we don't have something better that's public.

This comment has been minimized.

@cdeil

cdeil Jun 3, 2015

Member

You could add ~ a handful of parameters more to this function, e.g. the observatory name or the date range.

This comment has been minimized.

@mapazarr

mapazarr Jun 4, 2015

Member

Ok, I renamed it. I already have observatory for observatory name. As for other parameters, let's see if they are needed, and we can add them later.

# TODO: add column(s) for offset, and take it into account in the coord. transformation!!!
# RA, Dec
# random points on a sphere ref: http://mathworld.wolfram.com/SpherePointPicking.html

This comment has been minimized.

@cdeil

cdeil Jun 3, 2015

Member

There's a utility function for this already: https://gammapy.readthedocs.org/en/latest/api/gammapy.utils.random.sample_sphere.html
Should be possible to re-use it here?

This comment has been minimized.

@mapazarr

mapazarr Jun 9, 2015

Member

Done.

# alt, az
# TODO: should they be in the ObservationTable? (they are derived quantities, like dead time)
# TODO: since I randomized RA/DEC without taking into account the observatory, I'm getting negative altitudes!!!
# maybe I should randomize alt az, then transform to RA/DEC!!!

This comment has been minimized.

@cdeil

cdeil Jun 3, 2015

Member

Drawing random observation positions in Alt / Az with Alt > 0 deg and then computing RA / DEC for those seems like a good idea to me. More realistic and not not much work.

This comment has been minimized.

@mapazarr

mapazarr Jun 9, 2015

Member

Done.

# TODO: operate row by row (using Observation class) instead of by columns?
# TODO: general methods for filling obs tables run by run; this function should call it.

This comment has been minimized.

@cdeil

cdeil Jun 3, 2015

Member

Generally it should be preferred to have vectorized functions. If something can't be vectorised, then usually the right strategy is to put the Python for look as low as possible in the utility functions that are called.
But this can be tricky and sometimes it is the right decision to even have user-facing functions that are not vectorised.
Here I don't see any issue with using vectorised expressions everywhere?
We can talk about this tonight.

This comment has been minimized.

@mapazarr

mapazarr Jun 3, 2015

Member

I didn't understand what you mean. You mean to have a vector of observations (one component per row)? Oh, ok: you mean work with numpy arrays and the like. Done.

"""
n_obs_start = 1
astro_table = Table()

This comment has been minimized.

@cdeil

cdeil Jun 3, 2015

Member

Is there a reason why you can't just instantiate an ObservationTable here and then fill it?
Does it do some checking in the constructor that prevents that pattern of creating it?
If no, I think I'd prefer that (a bit simpler).

This comment has been minimized.

@mapazarr

mapazarr Jun 4, 2015

Member

Fixed.

@cdeil

This comment has been minimized.

Member

cdeil commented Jun 3, 2015

While you're working on this, could you pleas also update this observation table format spec / description?
https://gammapy.readthedocs.org/en/latest/dataformats/observation_lists.html

Don't worry too much about what's in runinfo.fits now ... the important thing is to come up with a good, documented format. Once that is written down we can adapt the existing file we have to this format.

@cdeil

This comment has been minimized.

Member

cdeil commented Jun 3, 2015

It would be nice if you could fake TELESCOPE_PATTERN (or at least N_TEL) and MUON_EFFICIENCY as well, these are probably parameters we want to use to group observations soon.

Otherwise 👍 , this is heading in the direction I had in mind when we talked about this.

@coveralls

This comment has been minimized.

coveralls commented Jun 4, 2015

Coverage Status

Coverage increased (+0.19%) to 43.32% when pulling 541c57c on mapazarr:gen_obs_tables into 0883871 on gammapy:master.

@mapazarr

This comment has been minimized.

Member

mapazarr commented Jun 4, 2015

I fixed the error on travis-ci. I will continue working on this tomorrow.

@mapazarr

This comment has been minimized.

Member

mapazarr commented Jun 4, 2015

@cdeil @mimayer
Do we need TSTOP and ONTIME? We can calculate ONTIME as TSTOP - TSTART, right? Or TSTOP as TSTART + ONTIME.

@coveralls

This comment has been minimized.

coveralls commented Jun 4, 2015

Coverage Status

Coverage increased (+0.34%) to 43.46% when pulling 895de1e on mapazarr:gen_obs_tables into 0883871 on gammapy:master.

@cdeil

This comment has been minimized.

Member

cdeil commented Jun 4, 2015

I'd say make TSTART required. You want to know the run date e.g. for time selection in findruns "give me all runs from 2005 to 2007".
TSTOP and ONTIME are sometimes of interest, so should be optional. You can still define them in this format, so that everyone will use the same names if they do add those columns, and not one guy calls it TSTOP and then another gal calls it TEND.

@cdeil cdeil added the feature label Jun 4, 2015

@cdeil cdeil added this to the 0.3 milestone Jun 4, 2015

from gammapy.utils.time import time_ref_from_dict #OK!
#from . import time_ref_from_dict
#from .. import time_ref_from_dict

This comment has been minimized.

@mapazarr

mapazarr Jun 4, 2015

Member

@cdeil which relative import should I use here?

This comment has been minimized.

@cdeil

cdeil Jun 4, 2015

Member

This should work:

from ..time import time_ref_from_dict

This comment has been minimized.

@mapazarr

mapazarr Jun 4, 2015

Member

Ok fixed. Thanks!

@@ -0,0 +1,21 @@
# Licensed under a 3-clause BSD style license - see LICENSE.rst
from __future__ import (absolute_import, division, print_function,
unicode_literals)

This comment has been minimized.

This comment has been minimized.

@cdeil

cdeil Jun 4, 2015

Member

Yes, please add this to any new file.

It's an Astropy coding guideline, and it also makes sense: if you don't add it, it is possible to get slightly different behaviour on Python 2 and 3 (e.g. 1/2 ==0 on Python 2 or 1/2 == 0.5 on Python 3). If test coverage is not very high and you don't put in the right test dataset, issues can go unnoticed.

I know it's annoying boilerplate code, but thinking about / discussing where you need or might need it in the future is worse than just pasting it into every file.

This comment has been minimized.

@cdeil

cdeil Jun 4, 2015

Member

We can remove it when we drop Python 2 support. I.e. when Sherpa supports Python 3 and > 90% of our users are on Python 3, i.e. maybe in a few years, maybe never. :-)

This comment has been minimized.

@mapazarr
@coveralls

This comment has been minimized.

coveralls commented Jun 4, 2015

Coverage Status

Coverage increased (+0.36%) to 43.48% when pulling bee18cc on mapazarr:gen_obs_tables into 0883871 on gammapy:master.

@mapazarr

This comment has been minimized.

Member

mapazarr commented Jun 5, 2015

@cdeil I started working on the doc about the format for obs lists. Please have a look at the "TODOs" in the code.

@coveralls

This comment has been minimized.

coveralls commented Jun 6, 2015

Coverage Status

Coverage increased (+0.35%) to 43.48% when pulling cd55c27 on mapazarr:gen_obs_tables into 0883871 on gammapy:master.

1234,24,2013-03-22 14:32,1832,83.7,-5.2,Crab Nebula,32
Usually it has many more columns with information about each observation. A list of Gammapy supported columns is::
OBS_ID: observation ID as an integer (starting at 0 or 1? does it matter?)

This comment has been minimized.

@cdeil

cdeil Jun 6, 2015

Member

Can you please re-format this as an RST simple table?
See here or here for a description of the format and here for an example of usage in the Astropy docs. Note that on any Astropy Sphinx docs page you can click "Page Source" at the very bottom and see the RST file ... this is sometimes easier to learn how to do something than to read the RST or Sphinx docs.

This comment has been minimized.

@cdeil

cdeil Jun 6, 2015

Member

(starting at 0 or 1? does it matter?) -> every experiment / observatory will do this as they like, no need to explain / define anything how OBS_ID is filled here, I think.

This comment has been minimized.

@cdeil

cdeil Jun 6, 2015

Member

The table should also have a Unit column, because in CSV there's no way to store the unit and something has to be defined.

This comment has been minimized.

@mapazarr

mapazarr Jun 9, 2015

Member

Done, done, done.

@@ -22,23 +22,48 @@ This has the advantage that everyone can work with whatever tool they like. Here
* `csvkit <https://csvkit.readthedocs.org/en/latest/>`_
* Your own script using e.g the `Python standard library CSV module <http://docs.python.org/2/library/csv.html>`_ or `pandas <http://pandas.pydata.org>`_
A run list must have at least a column called ``Run``::
A run list must have at least a column called ``OBS_ID``::

This comment has been minimized.

@cdeil

cdeil Jun 6, 2015

Member

Can you re-structure the whole page a bit?

Gammapy can work with observation tables in whatever format Astropy Table can read. So it could be CSV, but that doesn't have meta info, so at least for data distribution we'll use FITS (the current runinfo.fits).

So this general info about CSV tools should go to https://gammapy.readthedocs.org/en/latest/dataformats/file_formats.html#csv (if it's not all there already) and if you think it's useful, just keep a Sphinx :ref: link to that section from here.

This comment has been minimized.

@mapazarr

mapazarr Jun 9, 2015

Member

Done: (re)moved file format stuff.

OBS_ID: observation ID as an integer (starting at 0 or 1? does it matter?)
RA: pointing position right ascension in equatorial (ICRS) coordinates
DEC: pointing position declination in equatorial (ICRS) coordinates
AZ: average azimuth angle during the observarion

This comment has been minimized.

@cdeil

cdeil Jun 6, 2015

Member

typo: observarion -> observation (same in next line)

This comment has been minimized.

@mapazarr

mapazarr Jun 9, 2015

Member

Fixed.

AZ: average azimuth angle during the observarion
ALT: average altitude angle during the observarion
MUON_EFFICIENCY: average muon efficiency of the telescopes
TIME_START: start time of the observation stored as number of seconds after the time reference in the header

This comment has been minimized.

@cdeil

cdeil Jun 6, 2015

Member

I don't know if RST tables can nicely accomodate such long descriptions. If they automatically wrap to multiple rows or there's a way to do that, great. If no, please put a short description here and then a bullet list with longer explanations on some things below the table.

This comment has been minimized.

@mapazarr

mapazarr Jun 9, 2015

Member

Yes, they can.

TIME_STOP: end time of the observation in the same forma as TIME_START
TIME_OBSERVATION: duration of the observation
TIME_LIVE: duration of the observation without dead time
TRIGGER_RATE: average trigger rate of the system

This comment has been minimized.

@cdeil

cdeil Jun 6, 2015

Member

Things like trigger rate, temperature, quality should be optional.
Please add a column with flag "required" or "optional" to the table.
No idea for a good name for this column at the moment.

This comment has been minimized.

@mapazarr

mapazarr Jun 9, 2015

Member

I find it difficult to label the columns as required or optional.
Actually, we already said before that the only required column is OBS_ID.
I guess it depends on what do you want to do with the table afterwards. If you want to run an analysis, the only column needed is the OBS_ID: gammapy should be able to find the corresponding observation lists.
If you want to make a selection of runs (apply some filter, like findruns does), you need the columns that you want to filter on.
Plus, chances are that we label a column as optional, then implement some tool that works on it and forget to update the doc.
I added the column for now, but we should discuss this.

This comment has been minimized.

@cdeil

cdeil Jun 9, 2015

Member

This optional / required question is related to the question whether we have one or two classes representing observation tables / data store index tables. I still don't know what is best. If there's only one class, I agree that only OBS_ID should be required.

My suggestion would be you continue implementing functionality (obs selection and grouping) for a few days and then maybe it will become clearer what is best?

This comment has been minimized.

@mapazarr
Extra user defined columns are allowed; Gammapy will ignore them.
In order for the extra columns to have full meaning a header is needed with at least the following keywords::

This comment has been minimized.

@cdeil

cdeil Jun 6, 2015

Member

Again, please make the header keywords a table.

This comment has been minimized.

@mapazarr
TIME_REF_MJD_INT: reference time for other times in the list (i.e. TIME_START/TIME_STOP). Integer value in mean julian days.
TIME_REF_MJD_FRA: fraction of integer value defined in TIME_REF_MJD_INT.
TODO: change names already defined in event lists (and in `~gammapy.utils.time.time_ref_from_dict`) ``MJDREFI`` and ``MJDREFF``? Or adopt ``TIME_REF_MJD_INT`` and ``TIME_REF_MJD_FRA``?

This comment has been minimized.

@cdeil

cdeil Jun 6, 2015

Member

Yes, I think MJDREFI and MJDREFF is an existing standard or convention. We should re-use it here.

This comment has been minimized.

@mapazarr
TODO: change names already defined in event lists (and in `~gammapy.utils.time.time_ref_from_dict`) ``MJDREFI`` and ``MJDREFF``? Or adopt ``TIME_REF_MJD_INT`` and ``TIME_REF_MJD_FRA``?
TODO: should the observation list have already a header? Or should the header values be defined as columns, then interpreted correspondingly when stored into an ObservationTable? (I might be mixing up 2 things: obs lists and obs tables here? or should they have the same format?)

This comment has been minimized.

@cdeil

cdeil Jun 6, 2015

Member

I'm not sure what you mean here.

When ObservationTable is read from FITS it will have the FITS header in the .meta dict and this should be used.
When ObservationTable is read from CSV there will be no header info.
We should not try to store header key / vals in columns.

I realise there's very different things. A "run list" can be just a list of OBS_IDs. A data store index table can contain lots of information that will be needed for findruns. As I said, I don't have a full concept yet how best to implement classes that cover all the common use cases of things people do with observation lists for Gammapy.
Here my suggestion would be that you try to implement the common use cases as examples in the tests and see how it goes. Probably this will give clues as to which classes need to be added and if any should be removed.

This comment has been minimized.

@mapazarr

mapazarr Jun 9, 2015

Member

Ok. We can discuss tomorrow in the hangout how to proceed.

TODO: should the observation list have already a header? Or should the header values be defined as columns, then interpreted correspondingly when stored into an ObservationTable? (I might be mixing up 2 things: obs lists and obs tables here? or should they have the same format?)
TODO: should the observation list already define the times in this ``TIME_REF_MJD_INT + TIME_REF_MJD_FRA`` format? Or should it be converted once its read into an ObservationTable?

This comment has been minimized.

@cdeil

cdeil Jun 6, 2015

Member

I'm not sure in what format we should store times in observation tables.

  • MET (mission elapsed time) is a simple float, but needs the reference time to be meaningful, and it's not possible to store that in CSV files, so I guess is not a great choice.
  • MJD (modified Julian date) is a simple float and doesn't need the reference time, but maybe is not very readable.
  • UTC time string is longer, but maybe most readable and what user's want?

I'd say, maybe use UTC time strings (i.e. recommend some format, but accept any string the astropy.time.Time will take as input?

This comment has been minimized.

@mapazarr

mapazarr Jun 9, 2015

Member

Well, the variables are called "MJD" (MJDREFI, MJDREFF), so we can't just put UTC on top of it.
My table generator uses MJD as default, but has a debug option to show UTC.

================ ==========================================================================================================================================================================================================
keyword description
================ ==========================================================================================================================================================================================================
OBSERVATORY_NAME name of the observatory where the observations were taken. This is important for instance for coordinate transformations between celestial (i.e. RA/dec) and terrestrial (i.e. az/alt) coordinate systems.

This comment has been minimized.

@cdeil

cdeil Jun 11, 2015

Member

I would suggest to add a Required? column here as well.

OBSERVATORY_NAME is optional, it's not needed for many applications.

We will probably extend this to include things like the scheme and "chain", "version", "config" (see here), i.e. all the info that's needed to access the right files via the DataStore.

@mapazarr - I know improving data format documentation and code in gammapy.obs is a lot of work, but this observation and data management stuff is actually the bigger part of building background models from off runs. At the core building background models just means stacking and smoothing histograms filled from event-lists, i.e. this will not be 1000s of lines of code and it's OK and even needed for you to spend more time on the observation and data management handling.

This comment has been minimized.

@mapazarr
Extra user defined header entries are allowed; Gammapy will ignore them.
TODO: ``TEL_LIST``: since the final format is not clear, please mind having a look to this issue and its outcome:

This comment has been minimized.

@cdeil

cdeil Jun 11, 2015

Member

If there's an issue, you can remove the TODO from here.
Github issues have milestones which make sure things aren't forgotten.
But forgetting to remove this TODO from this RST page once that issue is adressed is not unlikely.

I use TODOs (mostly in code comments) sometimes for things that I think are too small to make Github issues for. They can still be found and addressed systematically via full-text search. But it's probably not a great way to work and I should stop doing that.

This comment has been minimized.

@mapazarr

mapazarr Jun 11, 2015

Member

Done. It was your suggestion to put it on the first place :-)
It's true that TODO's are not very nice, when looking into the code, but I still think they're useful so that people are aware of what is missing. It can help in debugging a problem in the analysis sometimes.

# build a time reference as the start of 2010
dateref = Time('2010-01-01 00:00:00', format='iso', scale='utc')
# TODO: using format "iso" for `~astropy.Time` for now; eventually change it

This comment has been minimized.

@cdeil

cdeil Jun 11, 2015

Member

I'm actually -1 on using the "fits" format, I think we should use "iso".
There's plenty of software (like python datetime or pandas) that can parse the iso format, the fits format is much less common.
The fact that we use UTC should be stated once in the docs and then it's OK.

This comment has been minimized.

@mapazarr
# in TIME_START and TIME_STOP
az = Angle(obs_table['AZ'])
alt = Angle(obs_table['ALT'])
if debug :

This comment has been minimized.

@cdeil

cdeil Jun 11, 2015

Member

PEP8 will give a warning here, there should be no space before the colon:
debug : -> debug:

This is not very important, but please run pep8 on this file to make sure your code formatting is OK.

This comment has been minimized.

@mapazarr

mapazarr Jun 11, 2015

Member

Done, done.

obs_table['RA'] = sky_coord.ra
obs_table['DEC'] = sky_coord.dec
# optional: it would be nice to plot a skymap with the simulated RA/dec positions

This comment has been minimized.

@cdeil

cdeil Jun 11, 2015

Member

If you want to do this, do it in a script in examples.
But it will never be done at this point in the code, so please remove this comment.

This comment has been minimized.

@mapazarr

mapazarr Jun 11, 2015

Member

Removed.

observatory_name='HESS'
n_obs = 10
obs_table = make_test_observation_table(observatory_name, n_obs)
assert len(obs_table) == n_obs

This comment has been minimized.

@cdeil

cdeil Jun 11, 2015

Member

Maybe add a few more asserts to check that positions and times are roughly as expected, i.e. that they were filled correctly?

This comment has been minimized.

@mapazarr
@@ -11,7 +14,7 @@
class Observation(object):
"""Observation (a.k.a. Run).
"""Observation (a.k.a. run).

This comment has been minimized.

@cdeil

cdeil Jun 11, 2015

Member

If you look here: https://gammapy.readthedocs.org/en/latest/obs/index.html#classes
you'll see that in the short description of the class the line is truncated:

Observation (a.k.a.

This is because Sphinx or the Numpy docstring Sphinx extension looks for a dot . when parsing the docstring.

So the one-line description should always be ended with a dot, and can't have dots inside.
So please rephrase here, e.g. """A single observation.""" and then explain later that observation is called run by some people if you think it's useful.
This class might go away, so it's not super-important.

I'm just mentioning this here because I want you to become a Gammapy / Python / Sphinx pro and over the coming weeks help clean up and improve the code and docs where-ever you see an opportunity for improvement. We can only achieve a really nice package if we keep re-factoring and polishing and we need your help with that.

This comment has been minimized.

@mapazarr

mapazarr Jun 11, 2015

Member

Ok, fixed.

* ...
convenience methods. The format of the observation table
is described in :ref:`dataformats_observation_lists`.

This comment has been minimized.

@cdeil

cdeil Jun 11, 2015

Member

This is actually not obvious to me: does this link work in the HTML docs?
(I don't have time to build the docs locally and check myself.)

This comment has been minimized.

@mapazarr

mapazarr Jun 11, 2015

Member

Yes it works (I had already checked). Nice, hu? :-)

This comment has been minimized.

@cdeil

cdeil Jun 12, 2015

Member

Yes, this is nice. One can even cross-link to Astropy or other doc sets via intersphinx.
I didn't know this a year ago, so there's plenty of places in Gammapy where one could cross-link high-level and API docs. Please do when you see opportunities for improvement or centralising duplicated documentation.

time_ref = time_ref_from_dict(self.meta)
time_ref_unit = time_ref_from_dict(self.meta).format
ss += 'Time reference: {} {}'.format(time_ref, time_ref_unit)
# TODO: for an unknown reason, I can't enable astropy.units.cds

This comment has been minimized.

@cdeil

cdeil Jun 11, 2015

Member

Let's try to reduce the number of new TODOs in Gammapy. The options are:

  • You try to figure it out and start using it.
  • Or, if it doesn't work, comment here what you've tried and want to achieve and I'll investigate.
  • You let it go because we don't really need this at all and remove the TODO.

This comment has been minimized.

@mapazarr

mapazarr Jun 11, 2015

Member

Removed. I found a workaround.

mjd_frac = 0.5
time_ref_dict = dict(MJDREFI=mjd_int, MJDREFF=mjd_frac)
time_ref = time_ref_from_dict(time_ref_dict)
decimal = 4

This comment has been minimized.

@cdeil

cdeil Jun 11, 2015

Member

This temp variable doesn't help, you can save one line and just pass this as a keyword argument in the next line:

assert_almost_equal(time_ref.mjd, mjd_int + mjd_frac, decimal=4)

This comment has been minimized.

@mapazarr
def test_time_relative_to_ref():
mjd_int = 500

This comment has been minimized.

@cdeil

cdeil Jun 11, 2015

Member

Again, the lines where you set mjd_int, mjd_frac and decimal below can be removed, just pass the values directly as keyword arguments.

The main reasons to define temp variables are:

  • You need the variable in many places in a piece of code
  • Expressions become too long and need to be split up in pieces
  • You pass values in somewhere in a function where you can only use positional arguments (i.e. have to use an un-pythonic, bad API) and not giving it a name reads to cryptic code (like you had for word / pix coordinate conversions).

This comment has been minimized.

@mapazarr

mapazarr Jun 11, 2015

Member

Ok. They were as analogy to the previous function, where I use them more than once.

@cdeil

This comment has been minimized.

Member

cdeil commented Jun 11, 2015

@mapazarr – I did one more review of this PR and left inline comments. Nothing major, you should be able to finalise this PR today or tomorrow.
If you're not sure what exactly to write about times in Gammapy, just write two or three sentences ("should use Time class everywhere, when storing times in files use UTC or MET with a reference time") now and then we'll update this in the coming months when the relevant CTA spec is written by Catherine Boisson and me.

Added a few more tests to the dummy obs table generator. Added doc ab…
…out times in Gammapy and a glossary. Fixed many small formatting isues.
@mapazarr

This comment has been minimized.

Member

mapazarr commented Jun 11, 2015

OK, I implemented all issues. @cdeil: I hope you can now merge the PR.
I also figured out how to put the doc in google drive. The link to the doc is here:
www.googledrive.com/host/0BzXHZQx0oCLBfmNwR294aFk5R0NuWlhTaDl5Q2Y0ajFJMGFXX0YxeW1CRlhjQ1FfelhFdEE/
It still is very slow during the upload, not because of the size of the docs dir (14 MB), but because of the amount of files (~1300). @cdeil @adonath: do you know if there is a chance to upload compressed to google drive dirs and expand them once they are uploaded?
Ok, I tried https://cloudconvert.com/ but it is still very slow because it has to upload the files into drive (it doesn't act inside it).
The idea with the tarball is that we split the work, so:

  • You don't have to install the branch and build the doc, just download and open the file.
  • I don't have to wait so long for the upload (I hope it doesn't break).

In any case, please let mo know if youcan see the contents of the link I gave you.

Time in Gammapy
***************
Gammapy adopts the exact same time standard as the Fermi Science Tools as described on this page: `Cicerone: Data - Time in Fermi Data Analysis <http://fermi.gsfc.nasa.gov/ssc/data/analysis/documentation/Cicerone/Cicerone_Data/Time_in_ScienceTools.html>`_.

This comment has been minimized.

@cdeil

cdeil Jun 11, 2015

Member

Wrap lines to ~ 80 characters also in RST files.

This section looks out of place, first formats, then time, then you list the formats sub-pages TOC:
https://5c0a1090d7f5da950a4c3a6c72e5dfd9c101960f-www.googledrive.com/host/0BzXHZQx0oCLBfmNwR294aFk5R0NuWlhTaDl5Q2Y0ajFJMGFXX0YxeW1CRlhjQ1FfelhFdEE/dataformats/index.html#time-in-gammapy

This comment has been minimized.

@mapazarr

mapazarr Jun 11, 2015

Member

I thought you wanted to have it there?
#278 (comment)
Please make a suggestion where should it go.
I wrapped the lines where it was possible: the tables in observation_lists.rst would be much less readable if I truncated the lines.

Returns
-------
`~astropy.time.TimeDelta` object with the time in seconds after the reference.

This comment has been minimized.

@cdeil

cdeil Jun 11, 2015

Member

Use the normal numpy docstring format for this return value.

This comment has been minimized.

@mapazarr

mapazarr Jun 12, 2015

Member

Fixed.

muon_efficiency_min = 0.6
muon_efficiency_max = 1.0
muon_efficiency = np.random.random(
len(obs_id)) * (muon_efficiency_max - muon_efficiency_min) + muon_efficiency_min

This comment has been minimized.

@cdeil

cdeil Jun 11, 2015

Member

I don't like multi-line expressions. They are hard to read. With this line break especially.
Please use a temp variable instead to avoid the line break.

This comment has been minimized.

@mapazarr

mapazarr Jun 11, 2015

Member

I also don't, but I also don't like breaking a simple expression into many lines of code with temporary variables: it makes the code very hard to read. This was the suggestion of autopep8 on long lines of code. I'd rather have it like this instead of with many intermediate steps.

This comment has been minimized.

@cdeil

cdeil Jun 12, 2015

Member

Coding style and what is more or less readable code is always a matter of taste.
For now I'm calling the shots in Gammapy and am defining the coding style.
I'll clean this up in #285.

Note that this is not unusual, as developers join and leave to work on projects they have to adapt to the existing coding style or the one maintainers aim for even if they don't like it, because consistency is important.

😄

obs_table['RA'] = sky_coord.ra
obs_table['DEC'] = sky_coord.dec
# positions

This comment has been minimized.

@cdeil

cdeil Jun 11, 2015

Member

What's up with this comment? Remove?

This comment has been minimized.

@mapazarr

mapazarr Jun 12, 2015

Member

Fixed.

@cdeil

This comment has been minimized.

Member

cdeil commented Jun 11, 2015

A few more very minor comments inline. I'll merge this tomorrow morning after travis-ci passes.

Yes, docs have lots of files and upload takes a few minutes. I think that's acceptable for large PRs where having the docs online helps.
As I said, I don't want to download the tarball ... if I have to get the docs locally I prefer checking out your branch via git and running python setup.py build_sphinx.

😴

@cdeil

This comment has been minimized.

Member

cdeil commented Jun 11, 2015

One more thing ... I see you've edited the comment above.
Github doesn't send email notifications for comment edits, so usually I won't see any extra things you write. It's better to make a new comment if you forgot to say something so that I'll see it.

@mapazarr

This comment has been minimized.

Member

mapazarr commented Jun 11, 2015

@cdeil I am aware of the error (warning) in the build_sphinx in travis-ci. I'm working on it.
One question:
I have a file called:
docs/api/gammapy.datasets.generate_observation_table.rst
that issues a warning in my local build of sphinx. It comes from the time when make_test_observation_table was called generate_observation_table. But I don't remember creating the file myself. Can I just delete it? Or how do I clean all the references to the old name?

@mapazarr

This comment has been minimized.

Member

mapazarr commented Jun 12, 2015

@cdeil I implemented the comments and fixed the warning in the sphinx doc. Could you please have a look at my concern in my previous comment?
Here is the new doc. BTW it doesn't take minutes to upload, buth rather ~ 1 h:
www.googledrive.com/host/0BzXHZQx0oCLBSGNKZDBZcnhlZzg/

cdeil added a commit that referenced this pull request Jun 12, 2015

Merge pull request #278 from mapazarr/gen_obs_tables
Generate observation tables

@cdeil cdeil merged commit 578a2b3 into gammapy:master Jun 12, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@cdeil cdeil changed the title from Generate observation tables to Document observation table format and improve gammapy.obs Jun 12, 2015

@cdeil cdeil changed the title from Document observation table format and improve gammapy.obs to Document observation tables and improve gammapy.obs Jun 12, 2015

@cdeil

This comment has been minimized.

Member

cdeil commented Jun 12, 2015

@mapazarr - Thanks for all your work on this and your first big PR for Gammapy being merged.

  • Forget about putting HTML docs online for code review then. I'll check them locally where needed.
  • I realise now that time handling in Gammapy is too complicated to discuss and address it as a side project in this PR. So I've created #284 and #285 where I'll continue this.

Probably the same will happen with (RA, DEC) <-> (ALT/AZ) <-> (DETX/DETY) coordinates.
It's complicated in detail, even if not much code needs to be written.
It's also needed for background modeling, which should be done in the tangential system and then background models reprojected onto sky images.
Please think about whether you want to learn about coordinates and take on the implementation and documentation of the FOV <-> Sky coordinate transformations and reprojection, or if that is something I should do. We can discuss and decide this in the Google hangout next week.

@cdeil

This comment has been minimized.

Member

cdeil commented Jun 12, 2015

Concerning your question about the old docs/api/gammapy.datasets.generate_observation_table.rst file, I've added an entry to the Gammapy developer docs here: https://github.com/gammapy/gammapy/pull/285/files#diff-00ab472a96028e9c3bcd77657d6c3701R15

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