[WiP] Add new GUI - Githubissues

LucasCampos commented 4 years ago

Thanks to the efforts of @NicoleEic , we now have a simple GUI for the project. Compared to her branch, the current proposal

Is integrated with pip
Has a different file structure (mostly to support the previous point)
Removed the background of the image
Removed some ellipses

The GUI itself is showing up on both Ubuntu and MacOS.

In order for me to be satisfied for the merge in the main branch, I would like to see the following tested

GUI shows up on Windows
It is possible to use the GUI to run all three types of analyses
- Searchlight
- Clusterized
- Full brain

Unfortunately I do not have the resources to do the testing here. @claudebajada I do believe you could test all of these in your local workstation.

claudebajada commented 4 years ago

The GUI works but it would be nice to have some sort of indication that the process has finished (this confused me for a while).

Also is the wb_view meant to load the data? The button is only loading the surfaces for me.

claudebajada commented 4 years ago

Could we also make the -cluster and -mask option mutually exclusive? I am not sure about how to design that.

NicoleEic commented 4 years ago

here my suggestions

indication that the process has finished: we could have a pop-up message box saying 'done'
wb_view should load all files with geig (or whatever the normalization was) in the specified folder. I can have a look why this might have gone wrong in your case
culster/mask option could be made mutually exclusive by having a sort of toggle button that allows only one to be chosen

claudebajada commented 4 years ago

my thoughts:

yes a pop-up would be perfect
thanks, yes that would be good if you could have a look
toggle is a good idea!

I don't have time to do these in the next few weeks, any of you could attempt. Otherwise I will pick it up as soon as possible!

KeithGeorgeCiantar commented 4 years ago

Hello, I just joined the team yesterday. I have carried out the required changes so that we can move forward with the GUI. The main feature that is left is to have the subprocess run in the background, so at to not block/freeze the app itself.

Here is how it currently looks on Windows: gui

LucasCampos commented 4 years ago

Hey, @KeithGeorgeCiantar. What is the current status of this issue? I have the impression the GUI is quite solid and usable already. I just tested it on MacOS, and all the features seem there, including non-blocking run.

At this point, I am ready to merge into main.

claudebajada commented 4 years ago

We were having issues with 4K displays but we decided to put it aside at the moment and deal with it some other time. Also, I think you wanted to push the bids branch before this but I don't think that will happen (we are having some trouble testing it).

From my end I am happy to merge.

@NicoleEic - do you have any thoughts before we merge? as you really spearheaded this at the brainhack

KeithGeorgeCiantar commented 4 years ago

I am happy with the current state of the code as well, and I think it is ok to merge.

LucasCampos commented 4 years ago

@claudebajada @KeithGeorgeCiantar

I am ready to merge this code into the master branch. Just to be double sure, you did test that the full brain analyses is running correctly, right?

KeithGeorgeCiantar commented 4 years ago

I did not test the full brain analysis to completion because I was informed that it takes a long time. I will start the test right now and let you know when it finishes.

claudebajada commented 4 years ago

The full brain will likely not run on you machine since it requires about 30GB of RAM.... I'm not in today but can hopefully test it tomorrow.

Thought I am unsure why this test would be needed as none of that code was touched for the GUI ... all we do is call the CLI app which has remained unchanged.

LucasCampos commented 4 years ago

@KeithGeorgeCiantar

As @claudebajada said, the issue is not only the duration of the computation, but the high RAM requirements. Considering the comment, I understand that you do not have access to the local supercomputing facilities, BSC-1. In that case, I would wait @claudebajada to be available to test it.

Incidentally, how does the GUI handle files with spaces? Does it automatically add quotes around these files? If not we have a bug in our hands.

@claudebajada

Because weird things happen. Maybe we have a weird unicode issue. Maybe something is misspelled. As there is no hard deadline, I would prefer to ship software that is tested, specially considering that once we upload to PyPi, it will be there forever. We can always bump the version up again, but I would rather not have that blemish.

KeithGeorgeCiantar commented 4 years ago

@LucasCampos Thank you for bringing up such an important point, I had taken it for granted that filenames would not include spaces. I will rectify the issue and push the changes as soon as possible.

LucasCampos commented 4 years ago

@KeithGeorgeCiantar. No issues! I am sorry for realizing it so late as well.

KeithGeorgeCiantar commented 4 years ago

So, I have fixed the problem with having spaces in filenames and file paths, and I also added the option to copy the command to the clipboard. This button will copy the command text without any newline characters and allow the user to paste it into the terminal. gui2

Up until now, the user could only copy the command from the text box, but since this contains newline characters, it would look like the image below. This would require some extra editing of the text, before pasting into a terminal. from_gui

With the new 'Copy to clipboard' button, the GUI pushes the latest version of the command text on the clipboard, and allows the user to paste the command which looks like the image below. This text would not need any extra editing and can run eaily.

@LucasCampos @claudebajada what are your thoughts on this?

claudebajada commented 4 years ago

Looks very good! I would also suggest we remove the "required arguments" and "optional augments" headings. This makes sense in the CLI where a user would have to choose which of the optionals to input, but in the current setup (in the GUI) all options are mandatory. The ones that are not needed are automatically hidden from the user.

KeithGeorgeCiantar commented 4 years ago

@claudebajada I have removed the sub-headings as suggested, and changed the top one to 'input arguments', as show below. gui3

I have also worked on a bug where the sub-process would continue to run, despite the user closing the GUI. So far it has worked on Windows, I will test it on my Linux VM and push the new changes.

KeithGeorgeCiantar commented 4 years ago

@LucasCampos @claudebajada I have tested the latest GUI changes on both Windows and Linux and everything seems to work as expected. The changes have been pushed so that you can test on your devices as well.

KeithGeorgeCiantar commented 4 years ago

On the suggestion of @claudebajada, I am making some small changes to the GUI, mainly with regards to widget ordering and colours. I have included a GIF to show the option of having a light mode and a dark mode, the final colours are still to be determined. light-dark

LucasCampos commented 4 years ago

@LucasCampos @claudebajada I have tested the latest GUI changes on both Windows and Linux and everything seems to work as expected. The changes have been pushed so that you can test on your devices as well.

Does this include the full brain analyses?

claudebajada commented 4 years ago

@LucasCampos yes, but we are now working on a few last niggly bits that we would like to fix before pushing ... hopefully be there soon

LucasCampos commented 4 years ago

Okay! Just send me the signal, and I will merge.

On Thu, 23 Jul 2020 at 12:41, claudebajada notifications@github.com wrote:

@LucasCampos https://github.com/LucasCampos yes, but we are now working on a few last niggly bits that we would like to fix before pushing ... hopefully be there soon

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/VBIndex/py_vb_toolbox/pull/18#issuecomment-662936903, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAU3Z7QO2WDTSXLBHCG3LN3R5AHU5ANCNFSM4ODI5ZDQ .

claudebajada commented 4 years ago

all pushed, the only thing that is needed is to add the new imports needed to the readme

KeithGeorgeCiantar commented 4 years ago

@claudebajada @LucasCampos I have already added the new imports to both the setup and the README, the only thing left is to update the README with a whole section on the GUI. However, I suggest that such a change to the README is carried out in a different pull request.

NicoleEic commented 4 years ago

Hi everyone, I'm really impressed at how nice the GUI looks! I have not been very active in the last few weeks, but if there is anything you need my input for, let me know.

VBIndex / py_vb_toolbox

[WiP] Add new GUI #18