ENH: Allow selecting projectors for reconstruction - #14220
Conversation
larsoner
left a comment
There was a problem hiding this comment.
Looks like a good start! Can you look to see where we currently use reconstruct_proj and update that tutorial or example? Or add the new version to some example or tutorial where it shows why it's useful?
|
@larsoner I will address these, but also I changed directions now so I will push a cleaner version now and then ping you (still draft + wrapping my head on how to approach this without breaking a lot of things/complex code) |
|
I also noticed ci launch with draft PRs, is that intended (I will add skip ci in the next commit) but maybe draft shouldn't launch CI only if asked? |
|
Some authors open in draft but want to see CI results / green-ness before marking as ready for review |
a091c07 to
eaa44f4
Compare
|
@larsoner I was not very happy with the initial implementation after I read it again, it applied the projection matrix directly inside This way I will keep working on the rest piece by piece, and I think I will do the 2nd part of the issue (optional rank and field) in a seperate PR to keep things clean and tidy.
Yes I will do this once I am done with the implementation |
larsoner
left a comment
There was a problem hiding this comment.
Still quite a bit of complexity added... but maybe it's unavoidable. Okay with me to proceed!
|
Another attempt to simplify it |
larsoner
left a comment
There was a problem hiding this comment.
Seems reasonable to me, I like the idea of moving the logic into apply_proj for applying a subset!
One tiny last equivalence test I'd like to see (unless I missed it, which is possible) would be something that can be done on main: create instance, add proj(s), apply it/them, then add more projs. This should give an equivalent object state to what you now support on this PR: create instance, add all projs, apply_proj on the same subset. I think in theory you should be able to do assert evoked_a == evoked_b, if not that then maybe object_diff, if not that, then checking ._data equivalence and _projector equivalence plus info["projs"] equivalence (active vs not, values, etc.) would work.
Doing this just on one object type (evoked) is enough I think, we should be able to trust our MixinAbstraction to get us to other objects.
I am leaning to 1+2. |
|
Yeah 1+2 would be good! |
|
Thanks @BabaSanfour ! |
Reference issue (if any)
First phase of and closes #14208.
What does this implement/fix?
Make projection reconstruction public
Additional information
Ideation with Sol 5.6 xhigh, implementation with help of 5.6 luna high. All code verified and double checked.