-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
XFit-style dipole fitting GUI #13074
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
base: main
Are you sure you want to change the base?
Conversation
|
The situation has improved somewhat. I've split-off some of the changes into a separate PR and the unit tests are now passing. We still need more unit tests, but hopefully this PR is becoming a little more manageable. |
|
Want me to review? (And push a commit if the circle failure is somehow related?) |
|
That would be great! |
|
Didn't get to it this week, might have time Monday. If not, then it'll probably be another week because next week is full of meetings for me! |
|
There's no hurry :) |
larsoner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty reasonable so far. Can you update + flesh out the top comment? For example I think towncrier is done and you have some unit tests, so not clear to me what is still missing.
I still need to actually do some manual testing to see how it works as well, which could be a next step (you can put it as a TODO in the top comment if you want 😄 )
| ) | ||
| parser.add_option( | ||
| "-b", | ||
| "--bem", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Someday we should maybe allow --sphere... I think MNE-C had some way to do this. Can you add a TODO comment in case someone wants to try?
|
@wmvanvliet this is in draft mode, is this good to go from your end? If so feel free to mark as ready, ping me, and I'll try actually using it and see if there are any show-stopping issues. CircleCI failure is real: so is its style complaint in case you want to test locally, I can replicate the example error with I'll look to see if I can see what docutils is angry about |
larsoner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uggghh when I run locally I get a reference cycle. Working on fixing it and will push if I figure it out
|
Fixed ref cycle (need weakrefs to all callbacks) and improved scraping to use GUI-style scraping |
| ), | ||
| ) | ||
| self._actors["sensors"] = list(sensors.values()) | ||
| self._actors["sensors"] = sum(sensors.values(), []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this magic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sum just adds things... and you can add lists to concatenate them. The default starting value is 0 though and that won't work for a list so you have to provide [] as the starting value. So
>>> sum([["a", "b"], ["c"]], [])
['a', 'b', 'c']
It's just a one-liner equiv for what you had before
self._actors["sensors"] = list()
for val in sensors.values():
self._actors.extend(val)
|
Only thing blocking this IMO is faster unit tests with better coverage. |
This adds a GUI to perform guided dipole modeling in the spirit of MEGIN's XFit program. This PR contains the base functionality needed to make the GUI useful. Useful enough to include in the next release of MNE-Python. The plan is to keep adding features in future PRs as well as some sorely needed speed improvements.
See here for a list of currently supported features: #11977
This PR depends on: #12071
Minimal example:
Todo: