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

show users how to delete drives #549

Open
jasmainak opened this issue Aug 19, 2022 · 17 comments · May be fixed by #613
Open

show users how to delete drives #549

jasmainak opened this issue Aug 19, 2022 · 17 comments · May be fixed by #613
Labels
docs Documentation and tutorials good first issue Good for newcomers
Milestone

Comments

@jasmainak
Copy link
Collaborator

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

@jasmainak jasmainak added docs Documentation and tutorials good first issue Good for newcomers labels Aug 19, 2022
@rythorpe
Copy link
Contributor

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

@jasmainak
Copy link
Collaborator Author

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

@rythorpe
Copy link
Contributor

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

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

raj1701 commented Mar 1, 2023

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

go for it.

@raj1701
Copy link
Contributor

raj1701 commented Mar 4, 2023

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 [WIP] ENH: add option to delete a drive #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
Copy link
Collaborator Author

jasmainak commented Mar 4, 2023

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

raj1701 commented Mar 5, 2023

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

rythorpe commented Mar 5, 2023

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

+1 for using chery-pick !

@raj1701
Copy link
Contributor

raj1701 commented Mar 6, 2023

Sure I'll go with cherry pick

@raj1701
Copy link
Contributor

raj1701 commented Mar 6, 2023

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 [WIP] ENH: add option to delete a drive #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
    image

@raj1701
Copy link
Contributor

raj1701 commented Mar 6, 2023

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

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
image

Please check this out

@jasmainak
Copy link
Collaborator Author

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

raj1701 commented Mar 8, 2023

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

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

@ntolley ntolley added this to the 0.5 milestone Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation and tutorials good first issue Good for newcomers
Projects
None yet
4 participants