Skip to content

Conversation

@larsoner
Copy link
Member

This PR should not change any outputs, but internally it:

  • Refactors _prepare_forward and _check_loose_forward to greatly simplify the logic they use, and deduplicate checks/flows across MNE and sparse solvers.
  • Adds ordered argument to pick_channels and pick_channels_forward, which is something that I have often wanted. Eventually we can probably simplify a lot of our code using ordered=True, as there are a lot of places we do the same picking-with-order logic instead of picking-as-set logic.
  • Fixes the CircleCI regression in master.
  • Deprecates the old way of using compute_depth_prior in favor of a cleaner, Forward-based method.
  • Adds an ignore for scanno in the inv round-trip test (should not matter, and I think our result is now possibly more correct because it resets scanno to match the number of entries in info)

These are important for #5881, and are hopefully the last steps we need (along with #5979) before having easy, unified, drop-in lead-field normalization across all solvers (!).

Ready for review/merge from my end @agramfort

@larsoner larsoner changed the title MAINT: Clean up and unify _prepare_forward MRG: Clean up and unify _prepare_forward Feb 27, 2019
@larsoner larsoner added this to the 0.18 milestone Feb 27, 2019
@codecov
Copy link

codecov bot commented Feb 27, 2019

Codecov Report

Merging #5999 into master will increase coverage by 0.01%.
The diff coverage is 86.46%.

@@            Coverage Diff             @@
##           master    #5999      +/-   ##
==========================================
+ Coverage   88.91%   88.93%   +0.01%     
==========================================
  Files         401      401              
  Lines       72816    72833      +17     
  Branches    12108    12108              
==========================================
+ Hits        64746    64773      +27     
+ Misses       5184     5179       -5     
+ Partials     2886     2881       -5

@larsoner
Copy link
Member Author

This pull request introduces 1 alert when merging 4ee9f7c into f7bb2ef - view on LGTM.com

new alerts:

  • 1 for Redundant comparison

Comment posted by LGTM.com

@larsoner larsoner merged commit 693bd2d into mne-tools:master Mar 1, 2019
@larsoner
Copy link
Member Author

larsoner commented Mar 1, 2019

Merged after chat with @agramfort

@larsoner larsoner deleted the prepare_forward branch March 1, 2019 19:55
@agramfort
Copy link
Member

Thx @larsoner

jeythekey pushed a commit to jeythekey/mne-python that referenced this pull request Apr 27, 2019
* MAINT: Working refactor

* MAINT: Refactor _prepare_forward

* STY: Flake

* FIX: Make LGTM happy?
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.

2 participants