Report optimizations #191
Report optimizations #191
Conversation
The code has been unreachable for years because only URIs with .h5 or .hdf5 extension were accepted for the report plugin.
hernando
force-pushed
the
report_optimizations
branch
from
0c99b30
to
51269b4
Compare
|
Build finished. |
brion/compartmentReportPlugin.h
Outdated
| @@ -45,16 +45,34 @@ class CompartmentReportInitData : public PluginInitData | |||
| * @param gids the neurons of interest in READ_MODE | |||
| * @version 1.4 | |||
| */ | |||
| explicit CompartmentReportInitData(const URI& uri, | |||
| const int accessMode = MODE_READ, | |||
| explicit CompartmentReportInitData(const URI& uri, const int accessMode, | |||
There was a problem hiding this comment.
no explicit; technically also now version 3.0
| { | ||
| if (_gids.empty()) | ||
| return _header.numCells; | ||
| else |
| @@ -662,29 +689,31 @@ bool CompartmentReportBinary::_parseHeader() | |||
| if (_tunit.empty()) | |||
| _tunit = "ms"; | |||
|
|
|||
| // After parsing the header we figure out the end of the mapping by reading | |||
| // the data offset of the first cell. | |||
| _dataOffset = get<uint64_t>(ptr, DATA_INFO + _header.headerSize); | |||
There was a problem hiding this comment.
why did this move out of _parseMapping()? It's always the same for multiple updateMapping() calls
There was a problem hiding this comment.
(I thought I had already answered this, but I can't see the comment)
Yes, in fact it should the data offset should be computable just with the information from the header, but it doesn't happen to be the case (or I didn't spend enough time figuring it out).
There was a problem hiding this comment.
still no satisfying answer why you had to move the code. Where it was before should still work, no?
There was a problem hiding this comment.
It can't be at the original place. The offset is needed for remapping the file (line 282) and then read the GIDs, which is done even if the full mapping is not processed.
| bool CompartmentReportBinary::_parseMapping() | ||
| { | ||
| const uint8_t* ptr = reinterpret_cast<const uint8_t*>(_file.data()); | ||
| size_t offset = _header.headerSize; |
|
Build finished. |
|
I also removed cellCount from the metadata. |
hernando
force-pushed
the
report_optimizations
branch
from
51269b4
to
f41d8f5
Compare
|
I made the change because even H5Gget_num_objs is too slow. |
|
Build finished. |
hernando
force-pushed
the
report_optimizations
branch
from
f41d8f5
to
909c409
Compare
As we discussed yesterday, I would propose to distinguish between 'required' and 'optional' metadata. |
|
Build finished. |
|
In fact, if I knew how to translate dict lookups in Python into function calls in C++ I wouldn't change the Python side. For the C++ side is not the same, as it doesn't provide the required language feature. |
|
ping |
A new implementation only constructor has been added to brion::CompartmentReport. This constructor can be used to request opening a report without parsing the mapping. Looking up the GIDs is also skipped if possible. The plugin interface has been extended to include the hint about parsing the mapping or not and a new function getCellCount has also been added. This allows to complete the report metadata without looking up the GIDs. The members compartmentCount and cellCount have been removed from CompartmentReportMapping because they cannot be computed efficiently in H5 reports. To replace them, CompartmentReportMapping::getFrameSize and CompartmentReport::getCellCount have been added. It also makes more sense to know the total frame size of a view rather than the full report. A potential bug in mapping parsing has been fixed in CompartmentReportBinary (a read operation was accessing an address beyond the current memory map).
hernando
force-pushed
the
report_optimizations
branch
from
909c409
to
52cd3f1
Compare
|
Build finished. |
No description provided.