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

Pythonic style #44

Open
leotrs opened this issue Aug 12, 2016 · 2 comments
Open

Pythonic style #44

leotrs opened this issue Aug 12, 2016 · 2 comments

Comments

@leotrs
Copy link

leotrs commented Aug 12, 2016

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 leotrs changed the title Current state Visualization module and pythonic style Aug 12, 2016
@aestrivex
Copy link
Owner

  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 leotrs changed the title Visualization module and pythonic style Pythonic style Aug 16, 2016
@leotrs
Copy link
Author

leotrs commented Aug 16, 2016

  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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants