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

Add functions to compute Schultz and Gutman Index #3709

Merged
merged 5 commits into from Jan 13, 2024

Conversation

jangwon-yie
Copy link
Contributor

It contains implementations of first, second kinds of Schultz index and their unit cases. Please give me any comments so that it will be merged to the main branch. Thanks.

@pep8speaks
Copy link

pep8speaks commented Nov 7, 2019

Hello @jangwon-yie! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 53:1: F403 'from networkx.algorithms.schultz import ' used; unable to detect undefined names
Line 53:1: F401 'networkx.algorithms.schultz.
' imported but unused

Comment last updated at 2019-11-07 11:57:18 UTC

Base automatically changed from master to main March 4, 2021 18:20
@dschult
Copy link
Member

dschult commented Dec 13, 2023

The Schultz index, and Gutman index (second type of schultz index) are related to the Wiener Index and Kirchoff Index (https://hrcak.srce.hr/132323). All four (and others) are used as markers of chemicals based on the molecular network of atoms connected by chemical bonds.

The functions provided here can be improved and updated. The file structure, doc_strings and tests seem pretty good. I think they should be moved into the module wiener.py and probably connected to the kirchoff index if we ever get that. Maybe they should all go into a distance module as suggested in #6975 but for now we can put these into wiener.py.

@dschult dschult changed the title Schultz index Add functions to compute Schultz and Gutman Index Dec 14, 2023
Copy link
Member

@dschult dschult left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll go ahead and approve this -- mostly to let you know that it is ready to be reviewed.
Do we want these functions? Is this the right place?

@ghost
Copy link

ghost commented Jan 7, 2024

The functions seem useful and appropriate for NX.
I would advocate to move all functions to Distance measures.

@dschult
Copy link
Member

dschult commented Jan 8, 2024

These measures involve both distances between nodes and the degree of the nodes. Can you say why you tink they should all be contained within the ditance_measures.py module?

@rossbar
Copy link
Contributor

rossbar commented Jan 9, 2024

IMO adding these actually motivates keeping a separate module for the related indices. From the user perspective it should be a moot point as users are encouraged to call indices from the top level namespace rather than go spelunking through the package hierarchy. In other words, it's largely an implementation detail that shouldn't hold anything up in this PR!

Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took the liberty of pushing up a few minor docstring tweaks, but otherwise LGTM. Thanks @jangwon-yie !

I'm certainly no expert, but I'm in favor of adding these indices - they're straightforward computations and seem to be generic measures of graph properties!

@dschult dschult merged commit fd3cadc into networkx:main Jan 13, 2024
39 checks passed
@jarrodmillman jarrodmillman added this to the 3.3 milestone Jan 13, 2024
cvanelteren pushed a commit to cvanelteren/networkx that referenced this pull request Apr 22, 2024
* Add implementaions of Schultz index of first, second kind and their unit tests

* Add schultz.rst and update __init__.py

* remove an ambiguous variable

* move new functions to wiener.py clean up references and docs

* Minor docstring touchups.

---------

Co-authored-by: Dan Schult <dschult@colgate.edu>
Co-authored-by: Ross Barnowski <rossbar@caltech.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

5 participants