Lst reader

[FrancaCassol]

This is a first version of the LST reader.
Since LST data files have both one CameraConfig table and many CameraEvent tables, I introduced two containers in the LSTCameraContainer in order not to refill the CameraConfig information at each event. Then, generic interesting data can be found in:

event.r0.tel[0]

and specific LST1 data can be found in

event.lst.tel[0].svc
event.lst.tel[0].evt

[dneise]

Okay, now the SST1M reader tests are failing, but this is expected since the SST1M reader does not yet support protozfits v1.0.2 which in turn is needed by the Nectarcam and LST.

I think it would be hard to expect from Franca to deal with the SST1M stuff. So I propose to mark these tests with xfail as explained here:
https://docs.pytest.org/en/2.9.0/skipping.html#mark-a-test-function-as-expected-to-fail

Until I have time to create a follow up PR for the SST1M stuff.

I believe at the moment, still nobody is using the SST1M reader ... @kosack would that be okay for you, if the SST1M reader is non functional for some time?

[kosack]

That's fine with me if sst1m is not working for a bit. So you can add xfails to the tests in this PR to get it working, and that should be ok for now. We can fix it after.

[dneise]

Franca fixed the problem with the SST1M reader so, that is fine now, no need for xfail-ing any tests. That's super. The tests are still failing, but "only" during:

travis-sphinx -v --outdir=docbuild build --source=docs/

with this message:

Warning, treated as error:
/home/travis/build/cta-observatory/ctapipe/docs/atmosphere/index.rst:35:toctree glob pattern 'index_*' didn't match any documents
Error: Error building sphinx doc

[kosack]

I would guess adding docs for atmosphere should not be part of this PR. However, to fix them, you should remove the index_* - that should just be a list of what sub-pages you have (they should not be named index_x.rst, since index implies the top page only.

[kosack]

Ideally, just move all the changes of atmosphere to a different pull request.

[FrancaCassol]

Actually I don't know why atmosphere is in this PR, I though it was included in a very old one (??) . I prefer to correct for it, removing the index_*

[dneise]

Ah Franka, now I remember. The same issue occured in the past on another PR from you.

I compared the master of your fork with the master of ctapipe, and indeed this atmosphere stuff exists in your master. So every feature-branch you branch off of your master like this:

git checkout -b <name_of_feature-branch>

and try to pull into the ctapipe master will carry this old modifications with it.

c.f.

master...FrancaCassol:master

[dneise]

To 'tidy up your fork' you can do

git checkout master
git fetch upstream
git reset --hard upstream/master
git push origin master --force

But please be aware this destroys the difference between your master and the upstream. c.f.

https://gist.github.com/glennblock/1974465

To tidy up this work here, I personally believe it is most easy and clean to start with a fresh branch and 'cherry pick' only the commits you actually wanted to commit.

Then open a new PR for that cleaned up branch and then close this PR.

[FrancaCassol]

Hi Dominik and Karl,
actually this stuff belong to an old pull that I was asked by Markus, clearly something went wrong with it (I was just starting with git and far from understanding how a pull works ;-) ). I can probably create a branch "atmosphere" where I keep the atmosphere changes in my master and delete them in my LSTreader branch, then I will eventually make a new pull with the atmosphere branch (after the pull of the LSTrader)... what do you think?

[dneise]

Yes, I believe, when you want to have the atmosphere stuff merged into the ctapipe master, you can open a dedicated pull request for that.

In order to do that, I recommend this:

git checkout master
# make sure your master is clean
git checkout -b atmosphere
git push -u origin atmosphere

then open the PR for the atmosphere branch. You can do that right now or later ... as you wish ... it is independent of this PR.

After you have successfully created the atmosphere branch and opened the PR for that, it is time to make sure your forks master branch is identical to the upstreams master branch as this greatly reduces the risk for accidentally including unwanted commits into new feature branches. So then I would recommend:

git checkout master
git fetch upstream
git reset --hard upstream/master
git push origin master --force

[codecov]

Codecov Report

Merging #749 into master will increase coverage by 0.42%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #749      +/-   ##
==========================================
+ Coverage   69.06%   69.49%   +0.42%     
==========================================
  Files         196      198       +2     
  Lines       10562    10708     +146     
==========================================
+ Hits         7295     7441     +146     
  Misses       3267     3267

Impacted Files	Coverage Δ
ctapipe/io/eventsourcefactory.py	96.55% <100%> (+0.12%)	⬆️
ctapipe/io/tests/test_lsteventsource.py	100% <100%> (ø)
ctapipe/io/sst1meventsource.py	96.87% <100%> (ø)	⬆️
ctapipe/io/nectarcameventsource.py	100% <100%> (ø)	⬆️
ctapipe/io/containers.py	100% <100%> (ø)	⬆️
ctapipe/io/lsteventsource.py	100% <100%> (ø)
ctapipe/coordinates/coordinate_transformations.py	86.95% <0%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b66d0b...b95fbcb. Read the comment docs.

[FrancaCassol]

Finally! I decided to remove the atmosphere directory, we don't need it right now, I can always pull it in when we think it starts to become useful (I will create a branch in my origin and keep it on the side)...

[FrancaCassol]

Hi Karl,

is stil there something wrong with this pull?

Thanks

Franca

[kosack]

is stil there something wrong with this pull?

No, looks good! I just was away and couldn't merge it. Will do it now.