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

BUG (GUI): Dipole plot scaling #717

Closed
gtdang opened this issue Feb 21, 2024 · 13 comments · Fixed by #730
Closed

BUG (GUI): Dipole plot scaling #717

gtdang opened this issue Feb 21, 2024 · 13 comments · Fixed by #730
Assignees
Labels
bug Something isn't working hnn-gui HNN GUI

Comments

@gtdang
Copy link
Collaborator

gtdang commented Feb 21, 2024

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
  • When any simulation is run or data from a txt file is loaded, they are selectable in both drop-downs (1) and (2). In Figure 2 we've run 2 simulations (Simulation 1 and 2) and loaded two data files (*_trial_S1_ERP...). This is regardless if they are simulation or measured data.
    Screenshot 2024-03-12 at 4 29 05 PM
    Figure 2

  • Options for smoothing (3) and scaling (4) only apply to data selected in (1).

    • This makes sense in the context of comparing simulation to observed data and you need to scale simulated data to match the observed.
    • But this is problem if the user wants to compare Simulation to Simulation (Figure 1). Or if the user selects loaded data in (1) and simulation in (2). Or loaded data to loaded data.

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 gtdang added hnn-gui HNN GUI bug Something isn't working labels Feb 21, 2024
@gtdang
Copy link
Collaborator Author

gtdang commented Mar 12, 2024

@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
Copy link
Contributor

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
Copy link
Contributor

ntolley commented Mar 13, 2024

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
Copy link
Collaborator

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

wondering if it would be simpler to:

  • get rid of the second dropdown
  • each "add plot" will plot only a single dipole (simulated or real)
  • allow users to add as many dipoles as they need with "add plot" button
  • make the plot interactive. User can shift + click to select two lines and it will show the RMSE.

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
Copy link
Contributor

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
Copy link
Collaborator

dylansdaniels commented Mar 13, 2024

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
Copy link
Collaborator Author

gtdang commented Mar 13, 2024

  • 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
Copy link
Collaborator

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
Copy link
Collaborator Author

gtdang commented Mar 13, 2024

Does it allow picking multiple objects though?

@jasmainak
Copy link
Collaborator

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
Copy link
Contributor

ntolley commented Mar 14, 2024

@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
Copy link
Collaborator

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

  • Type
  • Simulation data
  • Simulation Smooth window
  • Stimulation scale
  • Data to compare
  • Data Smooth window
  • Data scale

@jasmainak
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working hnn-gui HNN GUI
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

7 participants