netneurolab / netneurotools

Useful tools from the Network Neuroscience Lab
https://netneurolab.github.io/netneurotools
BSD 3-Clause "New" or "Revised" License
57 stars 33 forks source link

[ENH] Added dominance stats function #97

Closed liuzhenqi77 closed 3 years ago

liuzhenqi77 commented 3 years ago

I have the dominance stats snippet here! Alpha stage -- major changes are very welcomed!

liuzhenqi77 commented 3 years ago

Sorry seems lots of style issues 🤦‍♂️. I was using black but I disabled it.

rmarkello commented 3 years ago

Weird that black would cause issues... We're just using flake8 to check things, and I thought the two programs played nicely? Perhaps we're ignoring / enforcing different warnings / errors than your black settings are by default?

rmarkello commented 3 years ago

Ahhh, yes, it's mostly line length issues—that's likely just due to different settings. I'm willing to increase the max line length to 99 if you'd like (let me know), but then you'd still have a few line length errors and two other issues (unused variable and under-indenting).

liuzhenqi77 commented 3 years ago

Sorry I wasn't using black for this one, because it would change too many styles. Are you using any auto-formatting packages like autopep8 or manually correcting errors? (Btw, I think black is actually pretty nice, although I don't use it for my daily scripts.)

rmarkello commented 3 years ago

I have flake8 set up to run automatically on my IDE so it tells me if there are issues as soon as I type them—so, generally manually 😬

I'm willing to try and use black or autopep8 or something if you think that'd be preferable (though I disagree with some of their formatting choices sometimes 😂). What do you think? And, if you think it'd be good to do that, is there a way to set that up as a pre-commit hook or something?

liuzhenqi77 commented 3 years ago

Yes there seems to be ways to do this pre-commit, or using GitHub Actions, but I haven't used them. black might be better as it tries to be consistent (although I don't like some of the styles either.), but I don't have a strong preference. For this PR I'll manually correct them.

rmarkello commented 3 years ago

Looks like docs are failing now—primarily because you have an example that uses the original dominance_analysis toolbox which we don't want to have installed as a dependency. You should be able to add # doctest: +SKIP to those lines lines to get around that! (I think having them there for posterity is good so I wouldn't advocate completely getting rid of them :+1:)

Also there's the little netneurolab / netneurotools typo—happens to me all the time :joy:

liuzhenqi77 commented 3 years ago

Sorry seems I'm going to empty the github action time 😂

rmarkello commented 3 years ago

Sorry seems I'm going to empty the github action time 😂

Public repos get unlimited minutes 😎

liuzhenqi77 commented 3 years ago

Thank you for the review! Agree to all 🙌! I'm fully okay if next time you want to directly make changes , either to keep a consistent style, or to add best practices -- I'm all for it! (and will learn them!)

Re the assertion: it will raise when sum of computed total dominance is not equal to full model r square. This will not happen in normal cases, but if it happens, something weird must have happened (e.g. errors in the data or regression) -- more like a sanity check. I prefer to keep it, and I have added a message for it. Does this make sense?

I'm going to fix all and push a new commit (Is this the right way to do it?)

rmarkello commented 3 years ago

Hey @liuzhenqi77 ! Sorry I was AWOL on this for long (as you know, revisions took up most of my February 🙃)

This looks great though with the changes you last made, so I'm gonna go ahead and merge it in! Thanks again 🙌