Fix dl1 file naming to get rid of the stream number and "fits" extension #344
Conversation
|
Also, I had some troubles with the |
Codecov Report
@@ Coverage Diff @@
## master #344 +/- ##
=======================================
Coverage 43.42% 43.43%
=======================================
Files 68 68
Lines 4638 4644 +6
=======================================
+ Hits 2014 2017 +3
- Misses 2624 2627 +3
Continue to review full report at Codecov.
|
| from .r0_to_dl1 import get_dl1, add_disp_to_parameters_table | ||
| from .dl1_to_dl2 import * | ||
| from .volume_reducer import * | ||
| from .disp import * |
There was a problem hiding this comment.
Please no star imports without providing __all__. Neither editors nor code linters will be able to work properly (autocompletion / linting)
There was a problem hiding this comment.
I'm not sure what do you mean, all files provide __all__, but some functions were missing though, this is what you mean?
There was a problem hiding this comment.
This file does not have an __all__.
There was a problem hiding this comment.
ok, I get it. We need to modify all the __init__ files because none have __all__ and are making wildcard imports. Also modify some of the calls to avoid
Also, I had some troubles with the init because the file r0_to_dl1.py contains the function r0_to_dl1 (amongst others), then it cannot be included in the init as:
from .r0_to_dl1 import *
can we merge this one and I open an Issue/PR right afterwards to solve that in a different thread?
There was a problem hiding this comment.
so @maxnoe shall we move on with this PR and I'll correct the different __init__ files afterwards?
|
@maxnoe are you ok with the changes? |
rlopezcoto
changed the title
| 'dl1_' + os.path.basename(input_filename).rsplit('.', 1)[0] + '.h5' | ||
| ) | ||
| if (input_filename.startswith('LST')): | ||
| output_filename = ( |
There was a problem hiding this comment.
This looks like you are expecting a certain filename pattern here. Not just that the file starts with LST.
What is the format?
There was a problem hiding this comment.
Yes, names are e.g.:
LST-1.4.Run01975.0005.fits.fz
so:
'LST-' + tel_id + '.' + stream_number + '.Run' + run_number + '.' + subrun_number + '.fits.fz'
There was a problem hiding this comment.
So I think we should use a regex for this and check if this matches, I'll prepare a PR.
There was a problem hiding this comment.
And you want to remove the stream here? So dl1_LST-{tel_id}.{run_number}.{subrun_number}.h5'?
There was a problem hiding this comment.
No, that is the misleading thing about streams, see #301 .
The reader, if all 4 streams are in the same folder, reads the 4 of them and aligns the event numbers from the four files. So when you process the data in the standard way, event though the input string may be LST-1.1.Run01975.0005.fits.fz, the dl1 output has all 4 streams, that was the reason to get rid of the stream_number
There was a problem hiding this comment.
I agree that this may be problematic if you process only one of the streams, because you would lose the stream number in the output, but this is related to how the reader processes the data and in any case, you process all the 4 streams in most of the cases
There was a problem hiding this comment.
THis is what I wrote here, or not? The intention of the code is to add the dl1_ prefix, replace the extension with .h5 and to remove the stream number, correct?
There was a problem hiding this comment.
yes, that is right, but this is what is done in the code right now, right?
sorry, I read the original message that contained some stream_number in the output name
There was a problem hiding this comment.
AH yeah sorry, that was a typo I corrected immediately, but you probably read it in the mail
|
Is the stream number already removed in the master? From my latest test I think I'm still getting them: |
|
Grrr, I think that I know what happened there. The output file name is actually changed in: (addint the dl1_ already in the lstchain_r0_to_dl1 script). Then when it checks this:cta-lstchain/lstchain/reco/r0_to_dl1.py Line 193 in 6e3770e the file name does not start with 'LST' anymore... Will this get solved with #383 @maxnoe ? Otherwise I can work it out. |
|
It should be, yes. |
|
then let's wait for the merge |
Fixes #301
To get rid of the
.1.andfitsI had to include two different paths depending on the name of the file. Another option is to read the input file and treat it differently if it ismcorreal_data, but this one will work as long as we do not modify the naming of the files.