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

Change EventList from Table subclass to attribute #754

Merged
merged 8 commits into from Nov 18, 2016
Merged

Conversation

@cdeil
Copy link
Member

@cdeil cdeil commented Nov 4, 2016

This pull request:

  • Changes EventList from an astropy.table.Table sub-class to a class with an astropy.table.Table attribute.
  • Add EventList.select_row_subset method that dispatches to EventList.table and is called everywhere row subset selection occurs.
  • Changes GTI from an astropy.table.Table sub-class to a class with an astropy.table.Table attribute.
  • Add EventList test with a Fermi-LAT event list (make some IACT-specific columns optional, e.g. in print output)
  • Some fixes and improvement in EventListChecker
  • Adapts code using EventList
  • Misc cleanup and improvements of EventList related code
@cdeil cdeil added this to the 0.5 milestone Nov 4, 2016
@cdeil cdeil self-assigned this Nov 4, 2016
@joleroi
Copy link
Contributor

@joleroi joleroi commented Nov 8, 2016

Please add a test for stacking event lists

@cdeil cdeil force-pushed the cdeil:ev-list branch from 52d0297 to ccc016a Nov 10, 2016
@cdeil
Copy link
Member Author

@cdeil cdeil commented Nov 10, 2016

@adonath @joleroi I'm not done here adapting the code and test to the new EventList class, and the diff is already messy and hard to review.

But maybe you can have a quick look at this
https://github.com/cdeil/gammapy/blob/ccc016af65defcd3b1143b9826895a1effbbb112/gammapy/data/event_list.py#L73
it's basically this now:

class EventList(object):
    def __init__(self, table):
        self.table = table

Any comments?

@adonath
Copy link
Member

@adonath adonath commented Nov 14, 2016

@cdeil I've had a quick look at the changes and I don't have any specific comments. I think introducing the .table attribute is a good choice, because it cleans up the EventList API and removes inheritance complications with methods such as .read(), .write() and .info().

table = Table.read(str(filename), **kwargs)
return cls(table=table)

def info(self, file=None):

This comment has been minimized.

@joleroi

joleroi Nov 14, 2016
Contributor

Please change this to __str__ (and add an info method calling it if you like). see http://docs.gammapy.org/en/latest/development/howto.html#object-summary-info-string

This comment has been minimized.

@cdeil

cdeil Nov 18, 2016
Author Member

done

Copy link
Contributor

@joleroi joleroi left a comment

@cdeil Looks good from a very superficial look. Only one comment.

@cdeil cdeil force-pushed the cdeil:ev-list branch from ccc016a to d35efa6 Nov 18, 2016
@cdeil cdeil changed the title Improve EventList class and tests Change EventList and GTI from Table subclass to attribute Nov 18, 2016
@cdeil cdeil mentioned this pull request Nov 18, 2016
4 tasks done
@cdeil
Copy link
Member Author

@cdeil cdeil commented Nov 18, 2016

I've finished up this PR and updated the description above.
I'd like to make the 0.5 release today, so some tasks were moved to a future reminder issue: #782

@cdeil
Copy link
Member Author

@cdeil cdeil commented Nov 18, 2016

The only fail is this:
https://travis-ci.org/gammapy/gammapy/jobs/176966197#L2449
I'll merge this now, and update the notebook next.

@cdeil cdeil merged commit c3f14f0 into gammapy:master Nov 18, 2016
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@cdeil cdeil changed the title Change EventList and GTI from Table subclass to attribute Change EventList from Table subclass to attribute Nov 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants