Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fix] Fix leak in test_coregistration and attends various warnings #849

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

juangpc
Copy link
Collaborator

@juangpc juangpc commented Oct 6, 2021

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Oct 6, 2021

Codecov Report

Merging #849 (d118b7c) into main (fe015fd) will decrease coverage by 0.01%.
The diff coverage is 41.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #849      +/-   ##
==========================================
- Coverage   30.10%   30.08%   -0.02%     
==========================================
  Files         452      452              
  Lines       39254    39253       -1     
==========================================
- Hits        11816    11809       -7     
- Misses      27438    27444       +6     
Impacted Files Coverage Δ
libraries/disp3D/engine/view/view3D.cpp 0.42% <ø> (+<0.01%) ⬆️
libraries/mne/mne.cpp 1.42% <ø> (ø)
libraries/mne/mne_forwardsolution.cpp 20.70% <0.00%> (-0.14%) ⬇️
libraries/mne/mne_forwardsolution.h 3.22% <ø> (-8.54%) ⬇️
libraries/rtprocessing/icp.cpp 82.02% <ø> (ø)
testframes/test_filtering/test_filtering.cpp 100.00% <ø> (ø)
testframes/test_hpiFit/test_hpiFit.cpp 97.93% <ø> (ø)
...y_surface_set/test_mne_msh_display_surface_set.cpp 100.00% <ø> (ø)
libraries/inverse/hpiFit/hpifit.cpp 82.25% <50.00%> (-0.23%) ⬇️
...frames/test_coregistration/test_coregistration.cpp 94.54% <100.00%> (-0.10%) ⬇️
... and 4 more

// sourceEstimate,
// t_clusteredFwd,
// t_surfSet,
// t_annotationSet);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are the last two addSourceData commented out?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not used?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pRTDataItem might not be used but the data should still be added to p3DDataModel. Maybe change to:

p3DDataModel->addSourceData(parser.value(subjectOption),
                            "HemiLRSet",
                            sourceEstimate,
                            t_clusteredFwd,
                            t_surfSet,
                            t_annotationSet);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only see one addSourceData commented out. Where do you see the second one?

P3DDataModel is also not used any more. And this is the end of the application. But you're right, you never know. That is why it is not deleted.

But well. After reading your comment. It is true that there is a chance that there is some sort of communication between p3DDataModel and p3DAbstractView that makes this call to addSourceData needed. I was probably just hoping that not to be the case.

Done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe let me explain my point in a bit more detail. Ignore my comment about TWO addSourceData. You are right there is only one. If you comment out the addSourceData call, you will not add the computed source localisation result to the 3D view. That is the only problem I see. Regarding the return variable of addSourceData pRTDataItem, you can remove that one. It is not used in this example. But the call addSourceData is actually needed to visualise the result in 3D. Hope this clarified my problem a bit?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LorenzE Thanks for the comment. I've modified accordingly and pushed changes.

libraries/mne/mne_forwardsolution.h Outdated Show resolved Hide resolved
testframes/test_filtering/test_filtering.cpp Show resolved Hide resolved
@juangpc
Copy link
Collaborator Author

juangpc commented Nov 3, 2021

@LorenzE Please review this whenever you can.

t_clusteredFwd,
t_surfSet,
t_annotationSet);
// MneDataTreeItem* pRTDataItem = p3DDataModel->addSourceData(parser.value(subjectOption),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you comment out this call, the 3D view will be empty

@@ -227,7 +227,7 @@ int main(int argc, char *argv[])
AbstractView::SPtr p3DAbstractView = AbstractView::SPtr(new AbstractView());
Data3DTreeModel::SPtr p3DDataModel = p3DAbstractView->getTreeModel();
DigitizerSetTreeItem* pDigSrcSetTreeItem = p3DDataModel->addDigitizerData("Sample", "Fiducials Transformed", digSetSrc);
DigitizerSetTreeItem* pDigHspSetTreeItem = p3DDataModel->addDigitizerData("Sample", "Digitizer", digSetHsp);
// DigitizerSetTreeItem* pDigHspSetTreeItem = p3DDataModel->addDigitizerData("Sample", "Digitizer", digSetHsp);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't you want to visualise Digitizer date?

*
* @return clustered MNE forward solution.
*/
MNEForwardSolution cluster_forward_solution(const FSLIB::AnnotationSet &p_AnnotationSet,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why creating a new function instead of leaving the old one with the default parameters? Do you maybe have a good reference if this is best practice? Or is it just a personal preference?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants