Refactor get_all_functions to use inspect.signature instead of inspect.getfullargspe #104

akshitasure12 · 2025-03-30T13:37:08Z

This PR addresses issue #94

Changes made :

Replaced inspect.getfullargspec() with inspect.signature() for reporting the signature of the original function and not that of wrapped.
Updated the assertion logic in the test_get_chunks for chk_dict_vals to iterate over graph edges instead of nodes since chk_dict_vals consists of functions that deals with edges between the nodes and not the nodes themselves.

Schefflera-Arboricola · 2025-04-05T07:00:48Z

nx_parallel/tests/test_get_chunks.py

+            signature = inspect.signature(obj)
+            args = [
+                param.name
+                for param in signature.parameters.values()
+                if param.kind
+                in (
+                    inspect.Parameter.POSITIONAL_ONLY,
+                    inspect.Parameter.POSITIONAL_OR_KEYWORD,
+                )
+            ]
+            kwargs = [
+                param.name
+                for param in signature.parameters.values()
+                if param.kind == inspect.Parameter.KEYWORD_ONLY
+            ]


I checked the tests are passing(using pytest nx_parallel). The CI test failure is unrelated!

Maybe we can simplify this code by only returning kwargs instead of both args and kwargs as get_chunks is always a kwarg. Does that sound like a good idea to you?

Schefflera-Arboricola · 2025-04-05T07:09:53Z

nx_parallel/tests/test_get_chunks.py

+                        for edge in G.edges:
+                            assert math.isclose(
+                                c1.get(edge, 0), c2.get(edge, 0), abs_tol=1e-16
+                            )


Good catch! But this will now work with edge_betweenness_centrality and not betweenness_centrality because for the later the keys are nodes not edges. But, this seems like it is working because when you apply get on edges it always returns 0 for both c1 and c2. But, that shouldn't happen. So maybe we should use c1.keys instead of G.nodes(or G.edges). LMKWYT.

Schefflera-Arboricola · 2025-04-05T07:17:59Z

CONTRIBUTING.md

 - check-list for adding a new function:
  - [ ] Add the parallel implementation(make sure API doesn't break), the file structure should be the same as that in networkx.
-  - [ ] add the function to the `BackendInterface` class in [interface.py](./nx_parallel/interface.py) (take care of the `name` parameter in `_dispatchable` (ref. [docs](https://networkx.org/documentation/latest/reference/backends.html)))
+  - [ ] Include the `get_chunks` additional parameter. Currently, all algorithms in nx-parallel offer the user to pass their own custom chunks. Unless it is impossible to chunk, please do include this additional parameter.


Could you consider rebasing this branch -- so that these changes don't appear in the diff? Thanks!

https://youtu.be/DkWDHzmMvyg?si=bzbbgKiZDUppp1uC

akshitasure12 · 2025-04-06T19:15:10Z

I agree with you @Schefflera-Arboricola! I've made the changes you suggested.
Let me know if anything else needs to be addressed!

Schefflera-Arboricola

Thanks @akshitasure12 -- Apologies for the delayed response! Please LMK if you still have the bandwidth to address the following comments. Thanks!

Schefflera-Arboricola · 2025-04-24T21:34:11Z

nx_parallel/tests/test_get_chunks.py

+            kwargs = [
+                param.name
+                for param in signature.parameters.values()
+                if param.default is not inspect.Parameter.empty
+            ]


this is collecting all the default values of all the kwargs-- not the kwargs. Is that what you intended? The get_functions_with_get_chunks function calls this function-- and it assumes that get_all_functions is providing all the functions keyed to their kwargs. Could you please consider updating the docstrings of these functions to make it more clear to review?

We should also consider adding these helper functions to test/utils or utils maybe and then testing these as well

That was unintentional! I’ll get these changes done asap!

Schefflera-Arboricola · 2025-04-24T21:39:08Z

nx_parallel/tests/test_get_chunks.py

-                            assert math.isclose(c1[i], c2[i], abs_tol=1e-16)
+                        for key in c1.keys():
+                            assert math.isclose(
+                                c1.get(key, 0), c2.get(key, 0), abs_tol=1e-16


I don't think we should use .get here because the keys should be the same in both c1 and c2 and if they are not-- then an error should be raised instead of using 0. LMKWYT.

Schefflera-Arboricola · 2025-04-24T21:41:09Z

nx_parallel/tests/test_get_chunks.py

-                            assert math.isclose(c1[i], c2[i], abs_tol=1e-16)
+                        for key in c1.keys():
+                            assert math.isclose(
+                                c1.get(key, 0), c2.get(key, 0), abs_tol=1e-16


akshitasure12 · 2025-04-25T14:23:34Z

Thanks @akshitasure12 -- Apologies for the delayed response! Please LMK if you still have the bandwidth to address the following comments. Thanks!

Sure! I’ll be free in about a week and will get back to it.

…ons_kwargs() to return kwargs

Schefflera-Arboricola · 2025-05-06T15:41:09Z

nx_parallel/tests/test_get_chunks.py

+                            if not math.isclose(c1[key], c2[key], abs_tol=1e-16):
+                                raise ValueError(
+                                    f"Values for key '{key}' differ: {c1[key]} != {c2[key]}"
+                                )


no need for explicitly raising error like this here, assert already handles this

Schefflera-Arboricola · 2025-05-06T15:49:42Z

nx_parallel/tests/test_get_chunks.py

-def get_all_functions(package_name="nx_parallel"):
-    """Returns a dict keyed by function names to its arguments.
+def get_all_functions_kwargs(package_name="nx_parallel"):
+    """Returns a dict keyed by function names to its positional-or-keyword arguments.


why have you explicitly mentioned "positional-or-keyword"? -- it returns all the arguments. also in the following code could you please update the variable names i.e. update kwarg_?

All the functions only have positional or keyword arguments, so there is no need to mention that explicitly. I'll make the changes. Thanks!

…ded test for it.

Schefflera-Arboricola

Thanks @akshitasure12!
CI failures unrelated.

dschult

This looks like a good simplification and refactor.
And, of course, it now avoids the problems with getfullargspec.

I approve this PR

Update checklist (networkx#90)

21a4345

Schefflera-Arboricola reviewed Apr 5, 2025

View reviewed changes

Refactored tests and function logic

8d1fc59

Schefflera-Arboricola added the type: Bug fix Something isn't working label Apr 8, 2025

Schefflera-Arboricola linked an issue Apr 8, 2025 that may be closed by this pull request

BUG: get_all_functions() is not returning args and kwargs of the functions in algorithms #94

Closed

Schefflera-Arboricola reviewed Apr 24, 2025

View reviewed changes

akshitasure12 added 2 commits May 3, 2025 15:07

Improved error handling in test_get_chunks() and fixed get_all_functi…

f1c93db

…ons_kwargs() to return kwargs

Replaced c1.keys() with c1

480cabf

Schefflera-Arboricola reviewed May 6, 2025

View reviewed changes

akshitasure12 and others added 4 commits May 10, 2025 13:27

Updated the naming

4d57ce3

Merge branch 'main' into fix-for-issue-94

a061820

a few final touches

e496d45

moved get_all_funcs_with_args in get_functions_with_get_chunks and ad…

64149d0

…ded test for it.

Schefflera-Arboricola approved these changes May 10, 2025

View reviewed changes

Schefflera-Arboricola requested a review from dschult May 10, 2025 19:43

dschult approved these changes May 17, 2025

View reviewed changes

dschult merged commit 22e4c3c into networkx:main May 17, 2025
2 of 11 checks passed

Schefflera-Arboricola added this to the 0.4 milestone May 18, 2025

Uh oh!

Refactor get_all_functions to use inspect.signature instead of inspect.getfullargspe #104

Refactor get_all_functions to use inspect.signature instead of inspect.getfullargspe #104

Uh oh!

Conversation

akshitasure12 commented Mar 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Schefflera-Arboricola Apr 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akshitasure12 commented Apr 6, 2025

Uh oh!

Schefflera-Arboricola left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akshitasure12 commented Apr 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Schefflera-Arboricola left a comment

Choose a reason for hiding this comment

Uh oh!

dschult left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Schefflera-Arboricola Apr 5, 2025 •

edited

Loading