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

Add EventList plots #415

Merged
merged 21 commits into from Feb 10, 2016
Merged

Conversation

@JonathanDHarris
Copy link
Contributor

@JonathanDHarris JonathanDHarris commented Jan 10, 2016

This PR implements part of #409 .

A first implementation of plot_image used by the peek function.

I wanted to give you visibility early so you can tell me if I've got the wrong end of the stick.

Not sure about implementing the other plots, you mention some of them have have been done in #352 but I wasn't clear on (a) what's already been written (b) how I should call what's already been written.

@cdeil cdeil added the feature label Jan 15, 2016
@cdeil cdeil added this to the 0.4 milestone Jan 15, 2016
@cdeil cdeil self-assigned this Jan 15, 2016
import matplotlib.pyplot as plt
ax = plt.gca() if ax is None else ax

max_x = max((self[:]['COREX']))

This comment has been minimized.

@cdeil

cdeil Jan 15, 2016
Member

COREX / COREY is the reco event core position on the ground.
Use DETX / DETY here, which are the "field of view coordinates"
(I'll write up a spec what exactly those are soon here).

@cdeil
Copy link
Member

@cdeil cdeil commented Jan 15, 2016

Looks like a good starting point to me.

The next steps is to add this plot to the docs:
Add a file docs/data/event_list.rst (and reference it from the table of contents in docs/data/index.rst), and there put a plot directive that executes this method (see similar example here).
You can use the hess_events_simulated_023523.fits file from gammapy-extra using code such as here.

To make this work you have to clone the https://github.com/gammapy/gammapy-extra repo to your machine and set a GAMMAPY_EXTRA shell environment variable to point to the folder.
This is what the Gammapy code looks at to find it.

As the next plot, how about doing the time map mentioned in #409 ?

@JonathanDHarris
Copy link
Contributor Author

@JonathanDHarris JonathanDHarris commented Jan 15, 2016

Sure I'll probably get time to do the documentation and start looking at the time map over the weekend :)

@bsipocz
Copy link
Member

@bsipocz bsipocz commented Jan 24, 2016

@cdeil - The travis build will need a restart here as the current fails are unrelated and happen before the actual testing starts.

import matplotlib.pyplot as plt
ax = plt.gca() if ax is None else ax

max_x = max((self[:]['DETX']))

This comment has been minimized.

@cdeil

cdeil Jan 25, 2016
Member

Why did you put self[:]['DETX'] instead of just self['DETX'], i.e. index all the rows?
If the issue it that you need Numpy array instead of Column objects for the code to work, I'd suggest self['DETX'].data instead of [:].


count_image, x_edges, y_edges = np.histogram2d(self[:]['DETY'], self[:]['DETX'], bins=(x_edges, y_edges))

ax.set_title('# Photons')

This comment has been minimized.

@cdeil

cdeil Jan 25, 2016
Member

Please set axis labels.

This comment has been minimized.

@JonathanDHarris

JonathanDHarris Jan 25, 2016
Author Contributor

Sorry, what are the units of DET?

This comment has been minimized.

@cdeil

cdeil Jan 25, 2016
Member

DET is in degree

This comment has been minimized.

@JonathanDHarris

JonathanDHarris Jan 25, 2016
Author Contributor

Thanks

ax.imshow(count_image, interpolation='nearest', origin='low',
extent=[x_edges[0], x_edges[-1], y_edges[0], y_edges[-1]])

def plot_energy_dependence(self, ax = None):

This comment has been minimized.

@cdeil

cdeil Jan 25, 2016
Member

This is supposed to be a histogram of the energy values?
I think that would be nice to have, so either implement here, or put a TODO that it should be added later and raise NotImplementedError.

import matplotlib.pyplot as plt
ax = plt.gca() if ax is None else ax

def plot_offset_dependence(self, ax = None):

This comment has been minimized.

@cdeil

cdeil Jan 25, 2016
Member

Same here ... an offset histogram would be nice to have.

An energy-offset histogram will come soon, please don't try to implement it in this PR:
#383

--------
Plot a time map of the events:
.. plot::

This comment has been minimized.

@cdeil

cdeil Jan 25, 2016
Member

For the plot example from the docstring:

  • Remove unused imports (os, numpy, EventListDataset, EventListDatasetChecker)
  • I think this needs to be indented four spaces to work correctly? (does the plot show up in the docs if you run python setup.py build_sphinx -l?

This comment has been minimized.

@JonathanDHarris

JonathanDHarris Jan 25, 2016
Author Contributor

Thanks. I've fixed this up now. However, the plot looks a bit naff because it is not in loglog, and I couldn't work out how to call loglog for a subfigure (I'm currently calling it in peek() itself), so if you have any suggestions...


first_event_time = np.min(self[:]['TIME'])

# Note the events are not necessarily in time order

This comment has been minimized.

@cdeil

cdeil Jan 25, 2016
Member

You can assume that events are in time order.
We'll put this as a requirement for event list files.
No need to sort.
(did you ever see a file where events are not in time order?)

This comment has been minimized.

@JonathanDHarris

JonathanDHarris Jan 25, 2016
Author Contributor

Pretty sure the example there was not in time order....
It might have been me doing something silly in IPython, but I did run into a problem with it. Unless you're worried about performance, I would rather leave it in as defensive programming.

This comment has been minimized.

@cdeil

cdeil Jan 25, 2016
Member

My preference would be to remove this sorting, and to add a requirement to the event list spec that time must be sorted:
http://gamma-astro-data-formats.readthedocs.org/en/latest/events/index.html#required-column-names
(I've never seen an event list where times weren't sorted.)

If you prefer to be defensive, I'd suggest to not do the sorting or checking if sorted here in this plotting method.
There will be other code that relies on events being sorted if that's a requirement in the event list spec, and we shouldn't duplicate this in various places in Gammapy.

Probably the check should be added here:
https://github.com/gammapy/gammapy/blob/master/gammapy/data/event_list.py#L721
and then by default the checks could run on EventList construction once, not repeatedly while using the event list.
@JonathanDHarris - Does this sound OK to you? If yes, I could make a separate issue to remind us to implement this.

This comment has been minimized.

@JonathanDHarris

JonathanDHarris Jan 25, 2016
Author Contributor

Yep sounds sensible, I'll make an issue when I get home.

relative_event_times = np.sort(self[:]['TIME']) - first_event_time

diffs = np.array([relative_event_times[i]-relative_event_times[i-1]
for i in range(1, len(relative_event_times))])

This comment has been minimized.

@cdeil

cdeil Jan 25, 2016
Member

This can be expressed as a numpy array expression, which is less code and much faster than a Python list comprehension.

This comment has been minimized.

@JonathanDHarris

JonathanDHarris Jan 25, 2016
Author Contributor

Would this mean copying the array, shifting it, then subtracting it? Or have I missed something?

This comment has been minimized.

@cdeil

cdeil Jan 25, 2016
Member

Just using indexing and minus should work, no?

dt = time[1:] - time[:-1]

http://nbviewer.jupyter.org/gist/cdeil/f0b5c585bb78d7096a53

The minus operator will implicitly create a new array, dt ... no need for an explicit copy.

Does this make sense?

This comment has been minimized.

@JonathanDHarris

JonathanDHarris Jan 25, 2016
Author Contributor

Yep!

@cdeil
Copy link
Member

@cdeil cdeil commented Jan 25, 2016

The two plotting methods work already:

image

I've left a few inline comments.
Can you add a test that executes peek for the example event list file?

@cdeil cdeil changed the title First implementation of plot_image Add EventList plots Jan 25, 2016
@JonathanDHarris
Copy link
Contributor Author

@JonathanDHarris JonathanDHarris commented Jan 25, 2016

@cdeil I've been having trouble with the sphinx code. I don't have my local environment variables set up correctly for gammapy-extra, which doesn't help. Let me know if it's correct.

@cdeil
Copy link
Member

@cdeil cdeil commented Feb 4, 2016

I don't have my local environment variables set up correctly for gammapy-extra,

Could you please review #435 ? Does that info help you get gammapy-extra and the sphinx build working locally?

What do you see when you go to https://gammapy.slack.com/ ? Can you sign up or "apply"?

Sorry I haven't been responsive here!
I'll try out this PR and give feedback tonight or tomorrow morning latest.

@cdeil
Copy link
Member

@cdeil cdeil commented Feb 6, 2016

@JonathanDHarris - I tried this locally, and I get this plot:

image

Looks like there is an off by one error, resulting in negative time differences.
Can you reproduce / fix or should I debug and try to find the cause?

@cdeil
Copy link
Member

@cdeil cdeil commented Feb 6, 2016

This is what I currently see in the API summary:

screen shot 2016-02-06 at 11 51 58

Could you please add at least a 1-line docstring to plot_energy_dependence and plot_offset_dependence?
Maybe rename those to the somewhat shorter plot_energy_hist and plot_offset_hist or even plot_energy and plot_offset?
And add a plot_energy_offset that will use the recently added EnergyOffsetArray class.

@JonathanDHarris - You can just put raise NotImplementedError in any methods that you don't want to implement in this PR. If you do want to give the plot_energy_offset a shot, you'd have to rebase on latest master to get it. There are no merge conflicts, so it'll apply cleanly:

git fetch origin
git rebase origin/master
git push -f JonathanDHarris implement_peek_on_event_list
@cdeil
Copy link
Member

@cdeil cdeil commented Feb 6, 2016

@JonathanDHarris - Currently this test hangs and travis-ci fails:
https://travis-ci.org/gammapy/gammapy/jobs/104715046#L1247

gammapy/data/tests/test_event_list.py::test_Eventlist_peek 
No output has been received in the last 10 minutes, this potentially indicates a stalled build or something wrong with the build itself.

Locally I see a matplotlib GUI window pup up and I have to manually close it to resume the test.
I think the fix is to remove the plt.show() call in EventList.peek().
Can you please try running the test locally and see if that is / fixes the issue?

python setup.py test -V -t gammapy/data/tests/test_event_list.py
@JonathanDHarris
Copy link
Contributor Author

@JonathanDHarris JonathanDHarris commented Feb 6, 2016

I think in datasets/hess-crab4/hess_events_simulated_023523.fits the events appear out of order:

for i in range(1,L):
    if(event_list[:]['TIME'][i] < event_list[:]['TIME'][i-1]):print i, event_list[:]['TIME'][i], event_list[:]['TIME'][i-1]
   ....:     
200 155469968.526 155471655.068

@cdeil can you verify that, or is that a problem I've got locally?

@JonathanDHarris
Copy link
Contributor Author

@JonathanDHarris JonathanDHarris commented Feb 6, 2016

@cdeil Except for my comment above about time ordering events, I think the above commits address all your comments. I still need to double check though.

Your workaround for gammapy-extra does the trick for me, so I can check sphinx is building okay now, and the test no longer hangs.

@cdeil
Copy link
Member

@cdeil cdeil commented Feb 6, 2016

Your workaround for gammapy-extra does the trick for me

Good.

How do you launch Python for the case where it isn't working?
Ideally I'd like to figure that out and then include info in the docs over in #435.

I think in datasets/hess-crab4/hess_events_simulated_023523.fits the events appear out of orde

You are right!

Those are simulated events ... I'll try to track down the origin of that issue in the script that generates those event lists and we'll fix them so that the order is OK.

So for now, if you sort by time here, please keep a TODO comment that this should be cleaned up in the future...

@JonathanDHarris
Copy link
Contributor Author

@JonathanDHarris JonathanDHarris commented Feb 6, 2016

The API summary didn't seem to work, I'll look into it tomorrow.

@cdeil
Copy link
Member

@cdeil cdeil commented Feb 6, 2016

The API summary didn't seem to work

You probably have to use the -l option to run a "clean" Sphinx docs build:

python setup.py build_sphinx -l
@JonathanDHarris
Copy link
Contributor Author

@JonathanDHarris JonathanDHarris commented Feb 6, 2016

"How do you launch Python for the case where it isn't working?"
I'm just running python script.py

My PYTHONPATH appears to be blank as well, which might be something to do with it.

@cdeil
Copy link
Member

@cdeil cdeil commented Feb 6, 2016

@JonathanDHarris - Can you try this?

$ export KRONKA=LONKA
$ python -c 'import os; print(os.environ["KRONKA"])'
LONKA

I.e. for me, enviroment variables I set are available from Python.
Do you use bash?

@JonathanDHarris
Copy link
Contributor Author

@JonathanDHarris JonathanDHarris commented Feb 6, 2016

Yes, that worked for me. And yes, I use bash.

@JonathanDHarris
Copy link
Contributor Author

@JonathanDHarris JonathanDHarris commented Feb 6, 2016

I'll dig into it myself, it's probably something I missed.

@JonathanDHarris
Copy link
Contributor Author

@JonathanDHarris JonathanDHarris commented Feb 10, 2016

problem1
Sorry, I've had a look but I just can't figure out what is causing the problem above.

@cdeil
Copy link
Member

@cdeil cdeil commented Feb 10, 2016

@JonathanDHarris - For me locally the Sphinx docs method summary looks OK:

screen shot 2016-02-10 at 19 55 28

Not sure why you're seeing that issue.
Maybe try make clean and python setup.py build_sphinx again?
I think the issue should be gone then.
If not it could be that you have an old Astropy or Sphinx version.
In any case, you can also ignore that issue you see locally.

@JonathanDHarris
Copy link
Contributor Author

@JonathanDHarris JonathanDHarris commented Feb 10, 2016

If you're happy to ignore the docs, is there anything else left that needs doing?

@cdeil
Copy link
Member

@cdeil cdeil commented Feb 10, 2016

This looks good.
I'll merge as soon as the travis-ci tests are through.

I'll be using this to check HESS event lists in the coming days ... thank you!

cdeil added a commit that referenced this pull request Feb 10, 2016
…list

Add EventList plots
@cdeil cdeil merged commit e9d644d into gammapy:master Feb 10, 2016
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@JonathanDHarris
Copy link
Contributor Author

@JonathanDHarris JonathanDHarris commented Feb 10, 2016

Not at all, thanks for your patience with it.

@JonathanDHarris JonathanDHarris deleted the JonathanDHarris:implement_peek_on_event_list branch Jul 30, 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
You can’t perform that action at this time.