bctpy icon indicating copy to clipboard operation
bctpy copied to clipboard

Pythonic style

Open leotrs opened this issue 8 years ago • 2 comments

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

  • It seems the visualization module of BCT has not been ported to bctpy. Is this intentional? Is this planned for a future version?

  • Some names are not standardized across modules. For example, clustering.py uses clustering_coef_wd, where wd stands for weighted directed, whereas core.py uses assortativity_wei for weighted networks. I know this comes from the original BCT, but I think there's a point in making this package as good-quality as possible (and that includes consistent naming conventions).

  • Related to previous one. Is there interest in trying to make the package more pythonic, instead of a direct translation of BCT? For example,

    bctpy [master]$ flake8 . | wc -l
    546
    

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.

leotrs avatar Aug 12 '16 14:08 leotrs

  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.

aestrivex avatar Aug 16 '16 14:08 aestrivex

  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.

leotrs avatar Aug 16 '16 19:08 leotrs