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

Lst reader #776

Merged
merged 30 commits into from
Sep 21, 2018
Merged

Lst reader #776

merged 30 commits into from
Sep 21, 2018

Conversation

FrancaCassol
Copy link
Contributor

Added to LstEventSource class the possibility to read events from several files in parallel, this is necessary because the LST Event builder writes the events of a single run 4 four different fits files. The reader can now read them and loop on the events with the right order.

Since EventSource foresees only one input_url, in the case of multiple input files the name of the files must finish with suffix *000.fits.fz, *001.fits.fz, etc... and the user must give as input_url the name of the first file (*000.fits.fz). The program will search for the other files.
In the case of only one input file the input_url can have any form.

FrancaCassol and others added 26 commits September 28, 2017 12:47
…STCameraContainer (Fields: LSTEventContainer and LSTServiceContainer)
…le with version 1.0.2 of the protozfit library
@dneise
Copy link
Member

dneise commented Sep 4, 2018

@FrancaCassol I believe one would like to install the most recent version of protozfits for this:

https://github.com/cta-sst-1m/protozfitsreader/archive/v1.4.0.tar.gz

So environment.yml would need to be modified

@codecov
Copy link

codecov bot commented Sep 5, 2018

Codecov Report

Merging #776 into master will increase coverage by <.01%.
The diff coverage is 74.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #776      +/-   ##
==========================================
+ Coverage   71.54%   71.54%   +<.01%     
==========================================
  Files         199      199              
  Lines       10824    10878      +54     
==========================================
+ Hits         7744     7783      +39     
- Misses       3080     3095      +15
Impacted Files Coverage Δ
ctapipe/io/tests/test_lsteventsource.py 100% <100%> (ø) ⬆️
ctapipe/io/lsteventsource.py 88.54% <74.13%> (-11.46%) ⬇️

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 72d9b90...10a23a3. Read the comment docs.

@dneise
Copy link
Member

dneise commented Sep 6, 2018

Codacy is complaining about this:
https://github.com/FrancaCassol/ctapipe/blob/dbaa23c37a9d07c913ef5aa671b4ce919628fe57/ctapipe/io/lsteventsource.py#L218-L222

The thing is, line continuation with a backslash \ is discouraged by many style guides.
The reason I believe is, that a backslash at the end means something different depending on invisible trailing characters, so it is a line continuation if there is a line break after it ... but it is not a line continuation if there is a blank space after the \. Which is hard to spot and can easily lead to nasty bugs.

Most style guides I have seen, suggest to use implicit line continuation in brackets ().
So something like this:

# verify that the configuration_id of all files are the same
# in the CameraConfig table
for path in paths:
    assert (
        self._camera_config[path].configuration_id == 
        self._camera_config[paths[0]].configuration_id
    )

@dneise
Copy link
Member

dneise commented Sep 6, 2018

This is what pep8 writes about it

The preferred way of wrapping long lines is by using Python's implied line continuation inside parentheses, brackets and braces. Long lines can be broken over multiple lines by wrapping expressions in parentheses. These should be used in preference to using a backslash for line continuation.

Backslashes may still be appropriate at times. For example, long, multiple with-statements cannot use implicit continuation, so backslashes are acceptable:

@FrancaCassol
Copy link
Contributor Author

Thanks Dominik! I was indeed stuck on this point ;-)

@dneise
Copy link
Member

dneise commented Sep 6, 2018

No problem. I hope Codacy will be happy now ... :-)

@FrancaCassol
Copy link
Contributor Author

Hi Karl,
is there still something wrong with this pull?
Thanks

Franca

@kosack kosack merged commit d44863b into cta-observatory:master Sep 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants