jonescompneurolab / hnn-core

Simulation and optimization of neural circuits for MEG/EEG source estimates
https://jonescompneurolab.github.io/hnn-core/
BSD 3-Clause "New" or "Revised" License
55 stars 52 forks source link

BUG (GUI): Dipole plot scaling #717

Closed gtdang closed 7 months ago

gtdang commented 8 months ago

Description

Simulated and loaded data sometimes scale incorrectly when creating plots. Screenshot 2024-03-12 at 4 31 36 PM Figure 1

When creating data for dipole plots there are 4 relevant selections:

  1. Simulation Data
  2. Data to Compare
  3. Dipole Smooth Window
  4. Dipole Scaling

Solutions

  1. Limit the scope of drop-downs (1) and (2) to simulated data and loaded data, respectively.
    • This would prevent the user from making a direct comparison between two simulations. They can still plot two simulations on the same panel by over-plotting. (Selecting Simulation1 for (1) -> add plot -> Selecting Simulation2 for (1) -> add plot)
  2. Change (1) and (2) to generic "Data 1" and "Data 2". And add replicates of (3) and (4) that apply scaling and smoothing to Data 2.
gtdang commented 7 months ago

@ntolley @dylansdaniels Camilo has been looking into this issue and the solution will come down to what you think is the best use case for using and teaching with the GUI. Would you take a look at the updated description and proposed solutions and let us know what you think is the preferred option?

dylansdaniels-berkeley commented 7 months ago

Thanks for updating the description @gtdang!

I can see the argument for both, however, I feel like sticking to (1) would match best with our current workflow.

If we go for (2), the user would then have to toggle the smoothing/scaling on and off when appropriate, depending on what they're comparing. I could see this leading to similar issues / confusions as with the current method.

One potential issues with option (1), however, is how to stop users from loading whatever they want in a given dropdown anyway. What's to stop them from loading simulated data via the user data field if they're all just .txt files? We would need some way to disambiguate user data from simulations. Maybe we only allow simulated data to be loaded via HDF5?

If we go for (1), we could also try to make the use case of each drop-down field clearer by changing the naming conventions. Perhaps we go with something like "User Data" and "Simulated Data" instead?

ntolley commented 7 months ago

Perhaps @rythorpe and @jasmainak have a preference, but I tend to agree with Dylan that option (1) fits best with the workflow shown in the tutorials

As for loading in arbitrary files, I honestly think it's fine if they load in simulated data as through the "real data" field.

jasmainak commented 7 months ago

the main problem is that simulated and real data don't use the same scaling factor/smoothing.

wondering if it would be simpler to:

This last step could probably be done in hnn_core function for plotting dipole first and hopefully ipympl will translate that to the GUI seamlessly (to be tested/tried?)

rythorpe commented 7 months ago

Either @dylansdaniels-berkeley's or @jasmainak's suggestion sounds good to me. Regardless, users should be allowed to compare arbitrary waveforms, simulated or experimentally recorded, depending on their specific use-case.

dylansdaniels commented 7 months ago

Either sounds good to me as well. If we go with @jasmainak's approach, would the default be for scaling/smoothing be turned off since the tutorial workflow has users loading in data? Right now, each dropdown has a different behavior in that respect, so we would need to make a decision on how to handle that

gtdang commented 7 months ago
  • make the plot interactive. User can shift + click to select two lines and it will show the RMSE.

I think we'd need to introduce a plotly dependency to get this type of functionality. It's good idea and something we could look into but given the timeline this might need to be implemented after the May deadline we are trying to meet.

jasmainak commented 7 months ago

clearly plotly has done a good marketing job ;-) you can use basic matplotlib to do this kind of thing and ipympl should retain the interactivity (it's already a dependency). From chatgpt:

import matplotlib.pyplot as plt

# Generate some data
x = [1, 2, 3, 4, 5]
y1 = [1, 2, 3, 4, 5]
y2 = [5, 4, 3, 2, 1]

# Plot the lines
fig, ax = plt.subplots()
line1, = ax.plot(x, y1, label='Line 1', picker=5, color='blue')  # Set picker to a small value for precision
line2, = ax.plot(x, y2, label='Line 2', picker=5, color='red')   # Set picker to a small value for precision

# Function to handle mouse click event
def on_pick(event):
    if event.artist == line1:
        line_color = 'blue'
    elif event.artist == line2:
        line_color = 'red'
    else:
        return

    ind = event.ind[0]  # Index of the picked point
    x = event.artist.get_xdata()[ind]
    y = event.artist.get_ydata()[ind]
    print(f"You clicked on ({x}, {y}) of the line with color {line_color}")

fig.canvas.mpl_connect('pick_event', on_pick)

plt.legend()
plt.show()
gtdang commented 7 months ago

Does it allow picking multiple objects though?

jasmainak commented 7 months ago

Of course ... you'd have to track the selected objects in a list.

whatever solution you follow, I suggest that the logic in the GUI stays minimal ... it's supposed to be a thin wrapper on hnn-core. Otherwise we repeat the same mistakes as the old GUI.

ntolley commented 7 months ago

@jasmainak after some discussion with @gtdang I think we landed on just adding 2 sets of dedicated scaling/smoothing boxes for user data and simulated data

Invoking the same add plot button multiple times sounds nice code-wise, but after seeing how it works functionally it's really awkward to use when trying to adjust preprocessing

kmilo9999 commented 7 months ago

For this PR just add the 2 additional drop downs and reorder the list of options:

jasmainak commented 7 months ago

That sounds fair to me @ntolley ! Keep it short and simple