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

Auto generate tutorial data #530

Merged
merged 35 commits into from
Feb 5, 2024
Merged

Auto generate tutorial data #530

merged 35 commits into from
Feb 5, 2024

Conversation

CodyCBakerPhD
Copy link
Collaborator

@garrettmflynn Need your help integrating this with front-end

In the meantime, I'll continue debugging and extend it to multi-subject, multi-session, and multi-probe

@CodyCBakerPhD
Copy link
Collaborator Author

@garrettmflynn Might also need some help getting the test to trigger the endpoint properly, I keep getting a 415 error locally

@garrettmflynn
Copy link
Member

Getting an error about ModuleNotFoundError: No module named 'sklearn'. Are there additional dependencies here that weren't in the repo before?

@CodyCBakerPhD
Copy link
Collaborator Author

Ahhhh, yes sorry - I should have added them to the conda environment files

Through trial and error it will be whatever is needed to succeed with these imports: https://github.com/NeurodataWithoutBorders/nwb-guide/pull/530/files#diff-bfe22e422f7a3669b04d7ddfc8b73491eda63907f0a4da680bb6494f98f15d69R771-R773

@garrettmflynn
Copy link
Member

garrettmflynn commented Feb 1, 2024

Alright, I've updated this PR so that you can generate test data (and, at the same time, a dataset that replicates this data) on the Settings page. I've preemptively removed the Tutorial page since this replaces the data generation features there—though we don't currently have any textual material that replaces the original tutorial.

@CodyCBakerPhD Any suggestions before we merge this in?

@CodyCBakerPhD CodyCBakerPhD marked this pull request as ready for review February 2, 2024 15:54
@CodyCBakerPhD
Copy link
Collaborator Author

CodyCBakerPhD commented Feb 2, 2024

@CodyCBakerPhD Any suggestions before we merge this in?

Only to have a navigation popup button next to the 'delete' one that leads the user to the generated dataset so that they can see what it looks like after it completes

(I know it automatically pops up such a window when finished generating the first time - fine to keep that - but for renavigation would be nice to also have an easy button; also the initial popup took a while to actually launch on my system)

@CodyCBakerPhD
Copy link
Collaborator Author

Also, this PR removes autogeneration of the 'tutorial pipeline' in anticipation of the step-by-step screenshot approach approach, right?

@CodyCBakerPhD
Copy link
Collaborator Author

The delete command triggers a non-meaningful error when it tries to delete the target of the symlinks after the first time it was deleted (I assume that's what's going on); is there a way to 'safely' do that?

I also pushed a fix to the channel lengths on SpikeGLX, can you also run through a full test of a pipeline that uses these data?

@garrettmflynn
Copy link
Member

Can't get past the SourceData page with this error:
Screenshot 2024-02-02 at 3 10 47 PM

@garrettmflynn
Copy link
Member

The delete command triggers a non-meaningful error when it tries to delete the target of the symlinks after the first time it was deleted (I assume that's what's going on); is there a way to 'safely' do that?

Can you clarify what's triggering this error? Are you generating the dataset, deleting, regenerating, then deleting again? This is just removing the dataset output folder and shouldn't let you press the button without that folder existing.

@garrettmflynn
Copy link
Member

I've also found a fix for the lack of responsiveness when opening the folder location on Mac. Does this make things faster on your side as well?

@garrettmflynn
Copy link
Member

Can't get past the SourceData page with this error: Screenshot 2024-02-02 at 3 10 47 PM

Fixed this. Turns out that if I don't delete the data (only the dataset), the previously generated files will not be overwritten?

@CodyCBakerPhD
Copy link
Collaborator Author

I've also found a fix for the lack of responsiveness when opening the folder location on Mac. Does this make things faster on your side as well?

Mine no longer even opens at all; if possible, could you use the previous method if its Windows and your current method if its Mac?

Can you clarify what's triggering this error?

It was the kind of windows JS popup message that I can't copy/paste; said something about not being able to remove the resources because it didn't exist; I haven't seen it as of the last few commits

@garrettmflynn
Copy link
Member

Got it. I've ensured that the directories are only deleted if they exist, so that should solve the latter problem.

As for it not working on Windows, that's quite weird. Can you confirm that starting GUIDE again from the command line doesn't fix the issue? Since the changes were partially implemented on the main Electron process, the app needs to be restarted (which I also didn't notice and thought the system was broken after the initial implementation).

@CodyCBakerPhD
Copy link
Collaborator Author

As for it not working on Windows, that's quite weird. Can you confirm that starting GUIDE again from the command line doesn't fix the issue? Since the changes were partially implemented on the main Electron process, the app needs to be restarted (which I also didn't notice and thought the system was broken after the initial implementation).

That was it; fully restarting the app fixed the issue

@CodyCBakerPhD
Copy link
Collaborator Author

@garrettmflynn This is now working well for me; can you review, try it completely, and approve if happy with it?

Copy link
Member

@garrettmflynn garrettmflynn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great! Let's get this in.

@CodyCBakerPhD CodyCBakerPhD enabled auto-merge (squash) February 5, 2024 17:53
@CodyCBakerPhD CodyCBakerPhD merged commit 6f718cc into main Feb 5, 2024
10 of 11 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the generate_spikeglx branch February 5, 2024 17:54
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