-
Notifications
You must be signed in to change notification settings - Fork 27
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
Remove dependencies on neural-specific modules but issue a warning if a function is trying to be used that requires it #65
Comments
Sounds like a good idea (happy non-neural people are using the package!). Do you want to submit a PR? If not, I'll add this to the next version myself. |
|
So what I suggested what an awful suggestion at the end of the day. I think keeping it simple makes a lot of sense. How about this: Option 1 (you submit a PR).
Option 2 (I'll do it).Basically the same as 1, but I'll do the steps. The reason for doing option 1 is if you want the credit for the idea. |
I haven't dove too deep into the package but (in the future) having I'd like to take a shot at this. I'll let you know if I can't figure it out. I've been writing a paper non-stop lately so I could use a break haha. I've been meaning to learn how to properly implement decorators (you can see my failed attempt above) so this is the perfect tangent. btw, I do bioinformatics with an emphasis in microbial ecology (both environmental and human). |
Happy to hear that.
Only do it if you have time and want to contribute to the package. Otherwise I don't mind adding it myself. Your call.
It is probably a good idea to do |
I think I've implemented everything correctly (almost). Is there a test command that I should use? There's an error that I received from Was this class in another script at one point?
|
Fair point that that is confusing. "import bids" is importing pybids.
I think the best was to test would be to use: then you could do |
Thanks, for some reason I'm having trouble importing the new function that I added in
I thought |
If you submit a PR, it will be easier for me to look at the code/suggest changes. Hard to debug without seeing the code. |
It's been a minute since I've issued a pull request. Here's my repository: The pull request failed all tests as expect b/c of issues importing but I think once the import works it should pass the tests: When you get a chance, can you check out the imports of |
So spent some time looking at the problem this weekend. There is a circular import bug going on which I previously solved with a couple of absolute instead of relative imports (and the reason teneto was a little slow to import). It was basically all being held together with duck tape and something I sould have fixed a while ago. Now it is time to fix it. So I will solve this over the next few days cause there needs to be a little bit of restructuring of the code. And then we'll deal with your PR. |
Oh ok, I know how that is and those problems are the worst because it takes some significant rewrites. In the past what I've done was either brought everything into a separate utils package (e.g. teneto_utils) or brought everything in the util subpackage (e.g. teneto.utils). Good luck. |
Problems solved (mainly).
I will then go about and remove the dependencies and update documentation. I already tested the code, and it works fine. It just needs to be submitted correctly now. |
Sounds good! I'll try to get this done this week. I found the "optional" package error from
|
Just got it to work! Uploading now and issuing a pull request. Sorry for the delay. I had some papers come back that have been in review for a minute. Creating a new conda environment for teneto
Installin local version of
Making sure
Checking packages
|
Is there a test command I can run for |
Don’t worry. Should be fine. I’ll take care of this this evening. Sorry for it taking some time to get to.
/William
… 27 maj 2020 kl. 17:59 skrev Josh L. Espinoza ***@***.***>:
Is there a test command I can run for teneto to make sure eerything is working correctly?
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
Closing this as the PR is ready to be merged. |
As noted on the PR, after merging, this code has been removed. The reason were several:
Maybe you have an answer to these (primarily problem 2)? If yes, great! If you want to submit a new PR, please also change the .travis.yml file and add "pip install" commands for the neuropackages to the .travis.yml. This way we can run testing on the PR and not on the master branch like I did now (that is what I should have asked you to do at the start, sorry about that). |
Thanks for looking into this. I will see if there any adjustments I can make. Are there any tests I can run by hand to make sure everything imports and runs correctly before submitting the PR? |
The tests run automatically. It failed before cause of travis. But if you update the .travis.yml file with pip installs, then the tests will run
/William
… 1 juni 2020 kl. 17:43 skrev Josh L. Espinoza ***@***.***>:
Thanks for looking into this. I will see if there any adjustments I can make.
Are there any tests I can run by hand to make sure everything imports and runs correctly before submitting the PR?
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
Is your feature request related to a problem? Please describe.
No, but it would make this tool easier for broad scale usage
Describe the solution you'd like
Currently, there are several neural-specific package dependencies that may not be necessary for all researchers using this tool for temporal networks:
Would it be possible to not require
templateflow
,nilearn
, andpybids
for installation? For example, there could be a warning shown during installation that says for full functionality, please additionally install x, y, and z?Describe alternatives you've considered
This could also be implemented as a decorator that you could use for neural-dependent functions?
If I tried to use
function_dependent_on_neuralpackage
but didn't have the required packages it would tell me to installtemplateflow
,nilearn
, andpybids
.The code above is fully functional but I could help out with that if you were interested.
Additional context
Add any other context or screenshots about the feature request here.
The text was updated successfully, but these errors were encountered: