[REFACTOR] New RangeManager #5703
Conversation
| @@ -2984,7 +2972,7 @@ namespace OpenMS | |||
| } | |||
|
|
|||
| // update gradient if the min/max intensity changes | |||
| if (tmp.getIntensity() < current_layer.getFeatureMap()->getMinInt() || tmp.getIntensity() > current_layer.getFeatureMap()->getMaxInt()) | |||
| if (!current_layer.getFeatureMap()->getRange().RangeIntensity::contains(tmp.getIntensity())) | |||
There was a problem hiding this comment.
the namespaces look a bit strange with the getRange().RangeIntensity::contains ? is this intended?
There was a problem hiding this comment.
yes. Its a parent-class scope.
One could also add new methods to the RangeIntensity Class, i.e. containsIntensity(). That would avoid the need for the scoping, but its also more code to write in the RangeManager.
There was a problem hiding this comment.
I would say it would be a bit more in line with the getMinIntensity() etc. methods.
How much code would you need to add?
There was a problem hiding this comment.
one method per dimension. Four lines each.
I was acutally thinking about using an old-style preprocessor macro, just to avoid writing the same code 4 times (rt, mz, int, mobility). Not sure if thats the right way to go, but it would safe a lot of typing and testing.
There was a problem hiding this comment.
What do others think? I think will not write those rangemanagers that often and a nice interface would outweight the extra work here.
There was a problem hiding this comment.
@jpfeuffer what is your take on that?
There was a problem hiding this comment.
Might be better for compilation to have less code, which I am valuing very highly these days.
On the other hand this indeed pretty weird and I wouldn't be able to write that kind of code without looking up namespace rules or finding other usages for the first few times.
There was a problem hiding this comment.
Ah right :). @cbielow can you move some code from the header in the cpp file
There was a problem hiding this comment.
I can move some code, but what is moveable are mostly one-line functions.
The majority is templates and thus unmovable anyway.
I suspect compile time will not be affected in any way, but I can do it of course.
I also suspect that adding explicit functions will not influence compile time :)
So shall I add them?
…ame__ == '__main__':`. Avoids infinite recursion for spawning new processes
currently our pyopenms build will include any CPP it finds, even its it not in the build system... weird...
|
all tests pass. |
|
Chris will add namespace change to other PR |
Description
Replace the old RangeManager with a modern implementation, which supports more operations on Ranges (RT, m/z, Intensity, Mobility etc), and is safer to use, e.g. instead of
map.getMin()[0]which could mean anything, you now usemap.getMinRT().The new RangeManager supports arbitrary, but explicitly specified dimensions by using variadic templates, e.g.
RangeManager<RangeRT, RangeMZ>can manage RT and m/z dimensions.Ranges with different dimensions can interact, e.g. a range of a PeakMap (rt,mz,intensity) can be updated by the range of a spectrum (mz,intensity) by simply doing
map.extendRange(spec).This usually leads to a reduction in code size, and much cleaner code, e.g. see
https://github.com/OpenMS/OpenMS/pull/5703/files#diff-c819ce9062138032f71bd4408e255d4cd375069116192fcc9a01a6fbcf0f6656L314
RangeManager is implemented for all core datastructures, such as Spectrum, PeakMap, FeatureMap, ConsensusMap etc
and is used by the GUI Layers (further refactoring is needed there for the plotting canvas, which still uses old-fashioned DIntervalBase, but that change is rather large, and thus not part of this PR).
How can I get additional information on failed tests during CI:
If your PR is failing you can check out
Note: