Open sakshikukreja14 opened 3 years ago
Merging #1361 (3333a62) into develop (6325183) will increase coverage by
8.88%
. The diff coverage is50.25%
.
@@ Coverage Diff @@
## develop #1361 +/- ##
===========================================
+ Coverage 45.45% 54.33% +8.88%
===========================================
Files 58 56 -2
Lines 9889 9310 -579
===========================================
+ Hits 4495 5059 +564
+ Misses 5394 4251 -1143
Impacted Files | Coverage Δ | |
---|---|---|
src/core/libmaven/EIC.h | 75.00% <ø> (ø) |
|
src/core/libmaven/Fragment.cpp | 11.01% <0.00%> (ø) |
|
src/core/libmaven/Peak.h | 40.00% <0.00%> (ø) |
|
src/core/libmaven/PeakGroup.cpp | 67.40% <ø> (+0.90%) |
:arrow_up: |
src/core/libmaven/PeakGroup.h | 44.73% <ø> (-16.81%) |
:arrow_down: |
src/core/libmaven/PolyAligner.cpp | 0.00% <ø> (ø) |
|
src/core/libmaven/SRMList.cpp | 23.85% <ø> (+0.21%) |
:arrow_up: |
src/core/libmaven/classifier.cpp | 1.35% <ø> (ø) |
|
src/core/libmaven/classifierNeuralNet.cpp | 54.08% <ø> (-0.09%) |
:arrow_down: |
src/core/libmaven/csvreports.cpp | 24.40% <ø> (-11.97%) |
:arrow_down: |
... and 60 more |
@sakshikukreja14 A few comments from previous reviews have not been resolved because I do not see the requested changes. Do take a look.
@saifulbkhan sure will resolve the comments soon. Sorry, for missing out on some previously.
@sakshikukreja14 Adding another usability review for PeakML:
Peak detection dialog comes up with “Polly-PeakML” tab selected by default. Should be “Detection method”.
The “Preparing inputs for classification…” step is very slow for large peak-count. Is it the write-to-CSV part? I am on an SSD. Normal hard-disks can frustrate user. Eventually I force quit the application - not sure if this was just stuck or slow.
Tried the same dataset again in a fresh session and it worked very fast this time.
Sorting by label column does not work. Is that intentional?
Cycling through the “correlated peak-groups” worked well. But the currently selected peak-group in the peak-table was always the first entry in the correlation table. So if I cycle between masses 99, 100, 101 and 102 and the current mass selected is 101, then the order in the correlation table is 101, 99, 100, 102. Would be nice to have them always sorted by mass. The cycling itself is according to mass (which is the right behavior). So there is some inconsistency.
I edited one of the maybe peak-groups to include some of the missed peak areas and then marked it as “good” manually. Then pressed Cmd+z to see if undo is working. It worked the first time but was really slow (UI froze for about 5 seconds). Then I marked it as good again and then again tried undoing the marking this time it froze for 5 seconds again but did not undo my manually set “good” mark. Tried undo (pressed Cmd+z) one more time and then it worked. This was a table with less-than 200 peak-groups.
The force plot can do a little better when it comes to drawing labels for contributing attributes. In the example below, the markers are overlapping with the text (on the blue side) but there’s ample space to the right that can be used to position them without any overlaps.
I am able to select multiple peak-groups in the table and then explain their classification (but the force plot only comes up for the last selected group). We should disable the “Explain classification” menu item during multi-selection.
After relabeling the peak-table once or twice using new ranges, the context-menu (that comes up on right-click) for peak-groups no longer came up again for that table. It worked fine on any newly created table. Was not able to reproduce this bug again. Might be a one off case.
Everything else seems to be working as it should. The QA team might have some more feedback since they will probably play around with it more than I could.
Thank you @saifulbkhan for your reviews. Sorry, for the repeated crash. I will make the required changes soon.