[WIP] ENH,MAINT: Porting MNE Scan source plugins to MNE Analyze #901
Conversation
gabrielbmotta
changed the title
Codecov Report
@@ Coverage Diff @@
## main #901 +/- ##
==========================================
+ Coverage 30.20% 36.18% +5.98%
==========================================
Files 452 198 -254
Lines 39208 11812 -27396
==========================================
- Hits 11841 4274 -7567
+ Misses 27367 7538 -19829
|
| /** | ||
| * ForwardSolution Plugin | ||
| * | ||
| * @brief The forwardsolution class provides a plugin for computing averages. |
| MatrixXd matDataResized; | ||
| matDataResized.resize(iNumberChannels, data.cols()); | ||
|
|
||
| for(int j = 0; j < iNumberChannels; ++j) { |
There was a problem hiding this comment.
how about using more range based loops?
for(const auto& chName: lChNamesInvOp)
matDataResized.row(j) = data.row(lChNamesFiffInfo.indexOf(chName));
| * | ||
| * @param[in] parent The parent index. | ||
| */ | ||
| virtual int rowCount(const QModelIndex &parent = QModelIndex()) const override; |
There was a problem hiding this comment.
This is a general comment: In cases of classes which are final in their inheritance scheme, I would only keep the override keyword and remove the virtual keyword.
I do not think that ForwardSolutionModel will be used as a base class. Maybe consider marking the whole class as final.
| , m_bBusy(false) | ||
| , m_bDoRecomputation(false) | ||
| , m_bDoClustering(true) | ||
| , m_bDoFwdComputation(false) |
There was a problem hiding this comment.
Consider using in-class member initialisation instead of using the default constructor. The following core guideline is related. Your case does not 100% match the guideline example since you actually do something other than initialising members. Still I think it would make it more apparent to the reader of the class what the default values are.
|
|
||
| //============================================================================================================= | ||
|
|
||
| void ForwardSolution::init() |
There was a problem hiding this comment.
Try to avoid two phase initialisation -> https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#nr5-dont-use-two-phase-initialization
I know I probably introduced this a long time ago ;)
|
|
||
| void SourceLocalization::onModelRemoved(QSharedPointer<ANSHAREDLIB::AbstractModel> pRemovedModel) | ||
| { | ||
|
|
| //============================================================================================================= | ||
|
|
||
| FIFFLIB::FiffCov SourceLocalization::estimateCovariance(const Eigen::MatrixXd& matData, | ||
| FIFFLIB::FiffInfo* info) |
There was a problem hiding this comment.
can we maybe pass info as a const reference instead of a raw pointer?
| computedCov.data = finalResult.matData; | ||
|
|
||
| QStringList exclude; | ||
| for(int i = 0; i<info->chs.size(); i++) { |
There was a problem hiding this comment.
A range based loop would improve readability IMO
for(const auto& ch : info->chs)
if(ch.kind != FIFFV_MEG_CH && ch.kind != FIFFV_EEG_CH) {
exclude << ch.ch_name;
|
|
||
| void FwdSettingsView::showMeasFileDialog() | ||
| { | ||
| QString t_sFileName = QFileDialog::getOpenFileName(this, |
|
|
||
| void FwdSettingsView::showSourceFileDialog() | ||
| { | ||
| QString t_sFileName = QFileDialog::getOpenFileName(this, |
There was a problem hiding this comment.
can be const. Check code below as well.
Porting:
Adding MNE Analyze Data Models:
Display:
Debugging: