Speed up tdf import ~2.5x #1903
Conversation
- cache indices for a frame - increase scan package size - set number of threads manually
|
Ready for review now |
| public FrameSummedMobilogramProvider(Frame frame, int binWidth) { | ||
| this.frame = frame; | ||
| this.binWidth = binWidth; | ||
| this.binWidth = Math.max(1, binWidth); |
There was a problem hiding this comment.
binWidth is still number of data points right?
maybe you could add some java doc somewhere or even rename the var to dataPointsBinWidth?
|
|
||
| final List<BuildingMobilityScan> spectra = tdfUtils.loadSpectraForTIMSFrame(frame, frameTable, | ||
| scanProcessorConfig); | ||
| if (spectra.isEmpty()) { |
There was a problem hiding this comment.
function is nullable so need to check null or change to NonNull
| final List<BuildingMobilityScan> spectra = tdfUtils.loadSpectraForTIMSFrame(frame, frameTable, | ||
| scanProcessorConfig); | ||
| if (spectra.isEmpty()) { | ||
| spectra.add(new BuildingMobilityScan(0, new double[]{}, new double[]{})); |
There was a problem hiding this comment.
is that happening often? Then maybe create a constant for empty frames?
There was a problem hiding this comment.
no, just as a backup so it does not fail.
| * @param scanEnd The last scan index | ||
| * @return List of {@link SimpleSpectralArrays}, each represents the data points of one scan | ||
| */ | ||
| public List<SimpleSpectralArrays> loadDataPointsForFrame_v2(final long frameId, |
There was a problem hiding this comment.
maybe rename to loadDataPointsForFrameCached to define how it differs from the v1? but not needed.
You could also deprecate the old method and add some comment that v2 is faster?
| } | ||
| logger.info("Native TDF library initialised " + tdfLib.toString()); | ||
| setNumThreads(numThreads); | ||
| setNumThreads(1); |
There was a problem hiding this comment.
this always need to be 1?
The method is only called from this line so can it be multiple threads even?
There was a problem hiding this comment.
It can be, but its slower. It has to be set though.
| SimpleSpectralArrays data = extractCentroidsForFrame(frameId, 0, numScans); | ||
| SimpleSpectralArrays data = SimpleSpectralArrays.EMPTY; | ||
| if (msLevel < 2 || frameTable.getMsMsTypeColumn().get(frameIndex) | ||
| != 8) { // only load ms2 merged frame for non-pasef |
There was a problem hiding this comment.
maybe the !=8 could be extracted into a utils method like TDFUtils.isPasef(frameTable, frameIndex) or directly passing the type in.
Would be at least more descriptive
There was a problem hiding this comment.
good that you mention this, i wasnt sure if we should put this in at all. I'll remvoe it for now, it limits some possibities later on.
Need to chat with bruker if this works all the time before we mergen. also check if it makes sense to not import ms2 frame spectra, might have implications on other parts of mzmine