show users how to delete drives

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

show users how to delete drives #549

Open jasmainak opened 2 years ago

jasmainak commented 2 years ago

Fairly self-explanatory: it's currently not shown in our documentation how users can delete drives.

rythorpe commented 2 years ago

I thought we were trying to push users toward creating a whole new Network instance?

jasmainak commented 2 years ago

you can just do del net.external_drives[drive_name]. That's easier, no?

rythorpe commented 2 years ago

Sure, but the connectivity needs to be cleared as well. See here for an example. I know we discussed on numerous occasions the possibility of packaging all this in a simple net.remove_drive() function, however, it's my recollection that we opted for the solution that promoted a more ligthweight API: just recreate your network.

jasmainak commented 2 years ago

I think it's worth giving users a simple function if they can't do it non-trivially in a few lines of code.

See: #550

raj1701 commented 1 year ago

Hello @jasmainak @rythorpe, I am Rajat Partani. I wanted to apply for GSOC 2023 with HNN and am searching for a good starting point. If this issue is not resolved yet, can I work on it? I am fairly new to open source, I will be really thankful for your guidance.

jasmainak commented 1 year ago

go for it.

raj1701 commented 1 year ago

Hi @jasmainak, I had a few queries regarding what to do

There is a clear_drives function in network.py. Is documentation required for this. If yes can you please help me with the exact location where documentation needs to be updated
In #550 clear drivers is modified to delete only selected drives but was never merged. Is some work required there
In gui there is a button for deleting a drive. Is documentation required for that

jasmainak commented 1 year ago

I think #550 needs to be finished. It's basically a small function to remove the connectivity and the drive itself. But tests are remaining. The test should check that the drive is indeed removed. Look at the other tests and the contributing guide to see how we do tests. Can you start a new PR from the branch in #550 ?

raj1701 commented 1 year ago

Okay will work on the test section. If many rebase errors come up with #550 can I replicate the changes in the latest branch of master, write the tests and make a single PR?

rythorpe commented 1 year ago

If you're comfortable with cherry picking, I'd recommend branching from master and then cherry picking the single commit in #550. Then you won't have to worry about rebasing if you run into conflicts in the test files.

jasmainak commented 1 year ago

+1 for using chery-pick !

raj1701 commented 1 year ago

Sure I'll go with cherry pick

raj1701 commented 1 year ago

Hey @jasmainak @rythorpe , I did cherry picking and was able to get the commit. Only some typo errors came up in the function.

The function deletes all the external drives when no arguments are passed.
In test_network.py after clearing drives #550 asserts length of connectivity as 0 instead of 50. In the old code before that commit, assertion was made to 50 which seems correct.
Does this mean an external drive has multiple connectivities?
Also there is only a single test where all the drives are deleted. Should I add more tests for deleting a custom number of external drives and then checking the length of connectivity.
Here is a image where I have printed all values before and after running the function

raj1701 commented 1 year ago

Hey @jasmainak @rythorpe , I added some code to test custom delete drives In network.py In test_network.py

The driver names were correctly passed but self.connectivity shows a weird behavior. This time it became 0 after deleting all drives. Here are the print results

Please check this out

jasmainak commented 1 year ago

Does this mean an external drive has multiple connectivities?

You might want to read this: https://jonescompneurolab.github.io/hnn-under_the_hood/04_evoked-rhythmic-inputs/04_evoked-rhythmic-inputs

Should I add more tests for deleting a custom number of external drives and then checking the length of connectivity.

sure why not. But don't hard code the number of connections ... they should be computed somehow and intuitive to someone reading the code.

This time it became 0 after deleting all drives

Did you look what net.connectivity contains?

Also, please don't share screenshots of code. You can share code in markdown with proper formatting.

raj1701 commented 1 year ago

I will look into net.connectivity. In future will share code using proper practices. Also is one custom drive deletion test sufficient or should I add more? Currently I deleted 2 drives in the first go and remaining all in the second go

jasmainak commented 1 year ago

the tests should check all the possible scenarios and edge cases ... it doesn't matter how many you add