Closed seb5g closed 5 years ago
Hello, could you tell me what's wrong with this pull request (first time I do it...)?
Hi @seb5g, I missed your new PR. This looks good from a cursory view. I'll review this ASAP and dig why tests fail.
Hi @seb5g, I tested this PR locally and the unit test fail (see log below). I also tried to access your data file but the link does not work.
phconvert anto$ pytest -v
======================================================================== test session starts ========================================================================
platform darwin -- Python 3.5.4, pytest-3.0.4, py-1.4.31, pluggy-0.4.0 -- /Users/anto/miniconda3/bin/python
cachedir: .cache
rootdir: /Users/anto/src/phconvert, inifile:
collected 13 items
phconvert/test_bhreader.py::TestBhReader::test_import_SPC_150_nanotime PASSED
phconvert/test_pqreader.py::test_read_ptu_recordtype[filename0] PASSED
phconvert/test_pqreader.py::test_load_ptu[filename0] PASSED
phconvert/test_pqreader.py::test_read_ptu_recordtype[filename1] PASSED
phconvert/test_pqreader.py::test_load_ptu[filename1] PASSED
phconvert/test_pqreader.py::test_read_ptu_recordtype[filename2] PASSED
phconvert/test_pqreader.py::test_load_ptu[filename2] FAILED
phconvert/test_pqreader.py::test_read_ptu_recordtype[filename3] PASSED
phconvert/test_pqreader.py::test_load_ptu[filename3] PASSED
phconvert/test_pqreader.py::test_ptu_rtHydraHarp2T3_overflow_correction PASSED
phconvert/test_pqreader.py::test_ptu_rtHydraHarpT3_overflow_correction ^[[B^[[BPASSED
phconvert/test_pqreader.py::test_load_ht3 PASSED
phconvert/test_pqreader.py::test_load_pt3 PASSED
============================================================================= FAILURES ==============================================================================
_____________________________________________________________________ test_load_ptu[filename2] ______________________________________________________________________
filename = 'notebooks/data/Cy3+Cy5_diff_PIE-FRET.ptu'
def test_load_ptu(filename):
"""Test consistency of data loaded from PTU files."""
assert os.path.isfile(filename), 'File not found: %s' % filename
> timestamps, detectors, nanotimes, meta = phc.pqreader.load_ptu(filename)
phconvert/test_pqreader.py:88:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
filename = 'notebooks/data/Cy3+Cy5_diff_PIE-FRET.ptu', ovcfunc = None
def load_ptu(filename, ovcfunc=None):
"""Load data from a PicoQuant .ptu file.
Arguments:
filename (string): the path of the PTU file to be loaded.
ovcfunc (function or None): function to use for overflow/rollover
correction of timestamps. If None, it defaults to the
fastest available implementation for the current machine.
Returns:
A tuple of timestamps, detectors, nanotimes (integer arrays) and a
dictionary with metadata containing the keys
'timestamps_unit', 'nanotimes_unit', 'acquisition_duration' and
'tags'. The value of 'tags' is an OrderedDict of tags contained
in the PTU file header. Each item in the OrderedDict has 'idx', 'type'
and 'value' keys. Some tags also have a 'data' key.
"""
assert os.path.isfile(filename), "File '%s' not found." % filename
t3records, timestamps_unit, nanotimes_unit, record_type, tags = \
ptu_reader(filename)
if record_type == 'rtPicoHarpT3':
detectors, timestamps, nanotimes = process_t3records(
t3records, time_bit=16, dtime_bit=12, ch_bit=4, special_bit=False,
ovcfunc=ovcfunc)
elif record_type == 'rtHydraHarpT3':
detectors, timestamps, nanotimes = process_t3records(
t3records, time_bit=10, dtime_bit=15, ch_bit=6, special_bit=True,
ovcfunc=ovcfunc)
elif record_type in ('rtHydraHarp2T3', 'rtTimeHarp260NT3',
'rtTimeHarp260PT3'):
detectors, timestamps, nanotimes = process_t3records(
t3records, time_bit=10, dtime_bit=15, ch_bit=6, special_bit=True,
ovcfunc=_correct_overflow_nsync)
elif record_type in ('rtTimeHarp260NT2','rtTimeHarp260PT2'):
detectors, timestamps, nanotimes = process_t2records(t3records,
time_bit=25, ch_bit=6, special_bit=True,
ovcfunc=_correct_overflow_nsync)
else:
msg = ('Sorry, decoding "%s" record type is not implemented!' %
record_type)
raise NotImplementedError(msg)
acquisition_duration = tags['MeasDesc_AcquisitionTime']['value'] * 1e-3
ctime_t = time.strptime(tags['File_CreatingTime']['value'],
"%Y-%m-%d %H:%M:%S")
creation_time = time.strftime("%Y-%m-%d %H:%M:%S", ctime_t)
meta = {'timestamps_unit': timestamps_unit,
'nanotimes_unit': nanotimes_unit,
'acquisition_duration': acquisition_duration,
'laser_repetition_rate': tags['TTResult_SyncRate']['value'],
'software': tags['CreatorSW_Name']['data'],
'software_version': tags['CreatorSW_Version']['data'],
'creation_time': creation_time,
> 'hardware_name': tags['HW_Type']['data'],
'tags': tags}
E TypeError: list indices must be integers or slices, not str
phconvert/pqreader.py:103: TypeError
=============================================================== 1 failed, 12 passed in 15.44 seconds ================================================================
(py37-sm) phconvert anto$
Hello, I tried again with my ptu file: https://drive.google.com/file/d/1sMiY4SPt6_DFatCoRa0gMzERTmXMV37N/view?usp=sharing (sorry I deleted the previous one) and things are ok for me. Would you like to share the file you use for your testing? Compared to previous implementation of the reader, i noticed that some tags appears multiple times and so are better described as a list. Maybe it could be a reason...
You can run the unit tests with pytest -vx
, you see from the logs above that the failing file is Cy3+Cy5_diff_PIE-FRET.ptu
, tha t you can download from:
https://github.com/Photon-HDF5/phconvert/files/231343/Cy3.Cy5_diff_PIE-FRET.ptu.zip
All the other data files are on Figshare:
https://figshare.com/articles/Examples_data_files_for_phconvert/1455963
Diggin a bit more I found you opened a pandora box. There are a lot of repeated tags, including 'HW_Type' which is weird.
Either we convert all tag values to lists, and add [0]
whenever we use a single value or we leave this mix of list and other types and we add checks in cases such as HW_Type
(if list use element 0). The latter is easier if these duplicated case are not common but can become harder to maintain if cases like these will keep poppin up in the future.
Anyway, to print the tags with your patch you may want to use:
def _ptu_print_tags(tags):
"""Print a table of tags from a PTU file header."""
for n in tags:
tags_n = tags[n]
if not isinstance(tags[n], list):
tags_n = [tags_n]
for tag in tags_n:
if tag['type'] == 'tyFloat8':
value = f'{tag["value"]:20.4g}'
else:
value = f'{tag["value"]:>20}'
endline = '\n'
if tag['type'] == 'tyAnsiString':
endline = tag['data'] + '\n'
line = f'{tag["offset"]:4} {n:28s} {value} {tag["idx"]:8} {tag["type"]:12} '
print(line, end=endline)
@seb5g, I choose the latter approach, keeping a mix of list and other types for tags values, and I pushed the mods to your PR to fix the issue with HW_type. I also updated the _ptu_print function which was broken by tags containing a list.
Ok, so is there anything more I should do?
No, I am going to merge #37 soon, just left it in case you had comments. I just ask you what line you wanna add in the contributor's list in the README.
Added support to load phu files from picoquant (histograms) and ptu files from the timeharp. Added support also for tags that appears multiple times in the file (particularly for phu files). Test files and test code can be downloaded from: https://drive.google.com/drive/folders/1mdCoRq1nmgwaLASpPfmkRDDlMclJCQyx?usp=sharing
Regards Sébastien