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

WIP [Maint] Refactoring Hpi Fitting #884

Merged
merged 102 commits into from
Feb 23, 2022
Merged

WIP [Maint] Refactoring Hpi Fitting #884

merged 102 commits into from
Feb 23, 2022

Conversation

RDoerfel
Copy link
Contributor

@RDoerfel RDoerfel commented Jan 27, 2022

This PR aims to refactor the HPI class to make the code more readable, separate concerns, and make it easier to test. So far, no new functionality itself was implemented. I guess, most of the new lines are unit tests.

Following is a list of newly introduced classes.

HpiDataUpdater

Extracting important information like channel list from the FiffInfo. The used channels directly influence the data and projectors used for the HPI fit. However, this preparation based on the channel list has nothing to do with the HPI fitting algorithm and was therefore moved outside the HpiFit Class. Further, it provides the current SensorSet.

SensorSet and SensorSetCreator

SensorSet is a class containing the MEG sensor information and is constructed by SensorSetCreator. SensorSetCreator stores the link to the coil-definition file and a general FwdCoil object which contains information about all possible sensor types, and therefor, no information that should be carried by a specific SensorSet

HpiModelParameter

Is an object containing all the information (hpi freqs, sfreq, linefreq, number of coils,...) necessary to build the signal model and initiate an hpi fit.

SignalModel

The signal model is utilized by the HpiFit algorithm to compute the HPI coil amplitudes. Since building and fitting the model is a task in itself, it was moved to its own class. Further, a struct ModelParameters was introduced. This struct is used to construct the signal model and is also one of the Inputs for the HpiFit class. The struct contains the information necessary for the SignalModel and therefore the HpiFitting. It contains the Hpi Coil frequencies, the line frequency, and whether to use the advanced model or not (should be removed in the future).

HpiFit

The HpiFit class is now constructed using a SensorSet. Further, the number of input parameters was reduced, and the findOrder and fit functions were unified, since they do essentially the same, except 5 lines of code. Also, the results are now returned using the HpiFitResult struct.

Please find an example of the new usage of all these classes in the test_hpiFit Unit test.

ToDo

  • Check documentation of public functions. There is a lot of bad copy and paste right now
  • Check for Input Variable dimensions in HpiFit.fit()
  • rename ModelParameters to something like HpiModelParameters and make it an object
  • make SensorSet an object rather than struct (which is essentially an object in C++) to avoid direct access to important data.
  • make local variables const if necessary
  • add functionality to update the sensor set or create new HpiFit object when channels changed
  • improve handling of single hpiFit / continuous hpiFit and findOrder
  • add coil order function from @juangpc

Class Interaction Diagram

I hope the following makes the usage of the newly implemented classes a little transparent. Time flows from top to bottom.
Picture1

@codecov
Copy link

codecov bot commented Jan 27, 2022

Codecov Report

Merging #884 (ecb94a2) into main (bd31738) will increase coverage by 5.97%.
The diff coverage is 0.70%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #884      +/-   ##
==========================================
+ Coverage   30.20%   36.17%   +5.97%     
==========================================
  Files         452      199     -253     
  Lines       39208    11818   -27390     
==========================================
- Hits        11841     4275    -7566     
+ Misses      27367     7543   -19824     
Impacted Files Coverage Δ
libraries/inverse/hpiFit/hpifit.h 0.00% <ø> (-60.00%) ⬇️
libraries/inverse/hpiFit/hpimodelparameters.h 0.00% <0.00%> (ø)
libraries/inverse/hpiFit/sensorset.h 0.00% <0.00%> (ø)
libraries/inverse/hpiFit/signalmodel.h 0.00% <0.00%> (ø)
libraries/rtprocessing/rthpis.cpp 1.78% <0.00%> (-0.07%) ⬇️
libraries/rtprocessing/rthpis.h 0.00% <ø> (ø)
libraries/utils/file.cpp 0.00% <ø> (ø)
libraries/utils/ioutils.cpp 23.68% <0.00%> (-6.32%) ⬇️
libraries/utils/kmeans.cpp 0.27% <ø> (ø)
libraries/utils/kmeans.h 0.00% <ø> (ø)
... and 293 more

Copy link
Member

@LorenzE LorenzE left a comment

Choose a reason for hiding this comment

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

Very cool! Thx @RDoerfel

libraries/inverse/hpiFit/hpidataupdater.cpp Outdated Show resolved Hide resolved
libraries/inverse/hpiFit/hpidataupdater.h Outdated Show resolved Hide resolved
libraries/inverse/hpiFit/hpidataupdater.h Outdated Show resolved Hide resolved
libraries/inverse/hpiFit/hpifit.cpp Show resolved Hide resolved
libraries/inverse/hpiFit/hpifit.cpp Show resolved Hide resolved
libraries/inverse/hpiFit/sensorset.cpp Outdated Show resolved Hide resolved
libraries/inverse/hpiFit/signalmodel.cpp Outdated Show resolved Hide resolved
libraries/inverse/hpiFit/signalmodel.cpp Outdated Show resolved Hide resolved
libraries/inverse/hpiFit/signalmodel.h Outdated Show resolved Hide resolved
libraries/inverse/hpiFit/signalmodel.h Outdated Show resolved Hide resolved
@LorenzE
Copy link
Member

LorenzE commented Feb 7, 2022

@RDoerfel Do you mind creating a rough draft of the class hierarchy and how they play together? If yes, you can just add the image to the PR description. I think it would benefit understating the new structure.

…approach because of not-yet explained sign change
libraries/inverse/hpiFit/hpidataupdater.cpp Outdated Show resolved Hide resolved
libraries/inverse/hpiFit/sensorset.cpp Show resolved Hide resolved
libraries/inverse/hpiFit/signalmodel.cpp Outdated Show resolved Hide resolved
@LorenzE
Copy link
Member

LorenzE commented Feb 21, 2022

@juangpc @gabrielbmotta Can you have a look at this PR as well please? We would like to go ahead and start a new round of testing with this version. Thanks!

Also, any ideas why the Codecov is failing?

@gabrielbmotta
Copy link
Collaborator

Hi! The MEG is open on Friday for testing. We can merge as-is and then push any new changes after testing or test and incorporate changes to this PR. Whichever you prefer.

@juangpc juangpc merged commit cf25a47 into mne-tools:main Feb 23, 2022
@LorenzE
Copy link
Member

LorenzE commented Feb 24, 2022

@gabrielbmotta Thanks for having a look at this. The PR was still WIP and I think Ruben wanted to push some last commits. No need to reopen though, @RDoerfel can you just create a subsequent PR?

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