aestrivex / bctpy

brain connectivity toolbox for python
GNU General Public License v3.0
288 stars 137 forks source link

Pythonic style #44

Open leotrs opened 8 years ago

leotrs commented 8 years ago

Questions regarding possible contributions and how well they would be received.

There are 546 lines which deviate from flake8 styling guidelines. Is there interest in making bctpy adhere more to python standards?

If so, I would like to discuss the best way of setting up a PR.

aestrivex commented 8 years ago

1) The visualization module has been ported to bctpy. Check under utils. Certain functions were changed since the matlab plot3D had no straightforward equivalent so I implemented it with mayavi.

2) I have no strong opinion about the naming convention. Conservatively I copied the names in BCT. I would be open to changing them. For backwards compatiblity, it might be good to set up aliases for the old function names, after the new naming convention is established.

3) Adhering to various different style guidelines is not a high priority for me. I am open to PRs addressing style issues. I will happily receive any style changes that don't negatively affect the readability of the code at a glance.

leotrs commented 8 years ago
  1. Noted.
  2. There are five functions that compute the clustering coefficient:

    clustering_coef_bd(A)
    clustering_coef_bu(G)
    clustering_coef_wd(W)
    clustering_coef_wu(W)
    clustering_coef_wu_sign(W, coef_type='default')

    I propose defining just one dispatcher function, which can easily handle the first four cases.

    clustering_coef(A, weighted=False, directed=False)

    This function would just switch over its arguments and call the correct version of clustering_coef.

    In this way, we don't touch the current functions so we have full backwards compatibility, but we can expose and encourage the use of the new dispatcher in the future.

    The fifth case, clustering_coef_wu_sign(W, coef_type='default') can be supported by the dispatcher with one more optional parameter,

    clustering_coef(A, weighted=False, directed=False, coef_type='default')

    which will only be used in case weighted and directed are True. A little less clear than I would have hoped, but I think it still works.

    I propose this scheme for all other measures too. There are also four versions for transitivity, two for agreement, etc.

  3. I'm sure by adhering to PEP8 we won't incur in any readability blunders, all the contrary. I'll prepare an example PR soon.