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

Chore/style cleanup utility functions #80

Merged

Conversation

dPys
Copy link
Contributor

@dPys dPys commented Sep 4, 2024

Summary

This PR focuses on stylistic improvements and cleanup of utility functions.

Goal

Ensure better package maintainability, scalability, and ease of contribution for new developers.

Code Style Adjustments

  • Refactored variable names and formatting to improve readability.
  • Improved indentation, spacing, and overall syntax compliance with PEP8.

Utility Function Cleanup

  • Introduced @assign_algorithms decorator to dynamically assign algorithms to BackendInterface, reducing manual updates and boilerplate.
  • Simplified BackendInterface by replacing explicit algorithm declarations with automatic mapping from the ALGORITHMS list.
    • Even ALGORITHMS could be obviated by dynamically retrieving algorithm names directly from the modules using introspection (__str__, dir(), or __dict__), which might be too "auto-magical" for now but worth considering for a future PR.
  • Improved ParallelGraph's readability and consistency.
  • Reduced documentation and maintenance overhead through a more scalable, dynamic approach to algorithm registration.
  • Minor logic refactoring for efficiency.
  • Fixed bug in chunking:
    image

Removal of Unused Imports/Code

  • Eliminated unused imports, reducing code complexity and improving load time.

@dPys

@dPys
Copy link
Contributor Author

dPys commented Sep 13, 2024

@Schefflera-Arboricola -- all type annotations have now been removed. If there are no other issues, then it might be helpful for us to get this merged sooner rather than later so that I can begin working on some of the other features that we discussed. WDYT?

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 don't feel strongly about any of these comments, but these are some suggestions for choices during the style cleanup.

nx_parallel/interface.py Outdated Show resolved Hide resolved
nx_parallel/interface.py Outdated Show resolved Hide resolved
nx_parallel/interface.py Outdated Show resolved Hide resolved
nx_parallel/interface.py Outdated Show resolved Hide resolved
nx_parallel/utils/chunk.py Outdated Show resolved Hide resolved
nx_parallel/utils/chunk.py Outdated Show resolved Hide resolved
dPys and others added 2 commits September 14, 2024 13:12
Co-authored-by: Dan Schult <dschult@colgate.edu>
Co-authored-by: Dan Schult <dschult@colgate.edu>
@dPys
Copy link
Contributor Author

dPys commented Sep 14, 2024

I don't feel strongly about any of these comments, but these are some suggestions for choices during the style cleanup.

Thank you so much for the review! I've committed your formatting suggestions and agree with all of your comments.

@dPys dPys requested a review from dschult September 14, 2024 18:06
@dPys dPys requested a review from dschult September 22, 2024 00:33
@dschult
Copy link
Member

dschult commented Sep 23, 2024

What is the program that is creating the .history files? :)

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 think we're getting bogged down here in linting land... :(
Can we change the .gitignore file in a separate PR?

I like the changes here to the functions and the tests. Its the style changes that I'm having trouble with.

nx_parallel/tests/test_get_chunks.py Show resolved Hide resolved
nx_parallel/utils/chunk.py Outdated Show resolved Hide resolved
nx_parallel/utils/chunk.py Outdated Show resolved Hide resolved
nx_parallel/utils/chunk.py Outdated Show resolved Hide resolved
nx_parallel/utils/chunk.py Outdated Show resolved Hide resolved
nx_parallel/utils/chunk.py Outdated Show resolved Hide resolved
nx_parallel/utils/decorators.py Outdated Show resolved Hide resolved
Co-authored-by: Dan Schult <dschult@colgate.edu>
@dPys
Copy link
Contributor Author

dPys commented Sep 24, 2024

I think we're getting bogged down here in linting land... :( Can we change the .gitignore file in a separate PR?

Yes, happy to revert the .gitignore to main :) We can address this on a separate PR as well once this PR gets over the finish line.

@dPys
Copy link
Contributor Author

dPys commented Sep 24, 2024

What is the program that is creating the .history files? :)

The Local History extension of VScode creates .history folders to store automatic backups of your files so you can revert to previous versions, similar to how PyCharm's Local History works.

For now, I can say that if the preference is to be more conservative with what goes into the .gitignore file, then it is no problem for me to just use my local .git/info/exclude file which doesn't get committed to the repo :) With that said, the current .gitignore is missing quite a few common entries (.DS_Store being one of them, as it applies to all MacOS users)

@dPys dPys requested a review from dschult September 24, 2024 17:02
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.

This is basically ready to go as far as I'm concerned.
There are some comments below.

nx_parallel/utils/chunk.py Show resolved Hide resolved
nx_parallel/.DS_Store Outdated Show resolved Hide resolved
@dPys
Copy link
Contributor Author

dPys commented Sep 26, 2024

@MridulS -- it seems like removing the ruff dependency with that direct push to main is causing the style / format check to unconditionally fail for all PR's? Was that something that you were planning to revert?

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 approve these changes.
@Schefflera-Arboricola can you re-take a look?

@Schefflera-Arboricola
Copy link
Member

Schefflera-Arboricola commented Sep 26, 2024

@MridulS -- it seems like removing the ruff dependency with that direct push to main is causing the style / format check to unconditionally fail for all PR's? Was that something that you were planning to revert?

Can you try moving the update-get-info precommit hook at the end in .pre-commit-config.yaml file in this PR, because that hook internally runs a ruff command and @MridulS removed the ruff dependency. But, I think if we run the ruff hook before update-get-info hook then ruff would be installed in the pre-commit environment for the update-get-info hook to use. I think this should resolve the failing style test. Also, I did the same thing in the PR that @MridulS made but that PR is made from the main branch of nx-parallel so i think that's the reason the style test is passing for the push but not for the pull_request. (PR #84)

for future refernce - CI error msg : _nx_parallel/script.sh: 5: ruff: not found

Copy link
Member

@Schefflera-Arboricola Schefflera-Arboricola left a comment

Choose a reason for hiding this comment

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

Thank you very much @dPys and @dschult :)

I'm merging this even though the style CI is failing. We can handle that in another PR. This is good to go I think. Thank you again @dPys for putting this together and being so patient :)

@Schefflera-Arboricola Schefflera-Arboricola merged commit 3ebd2a8 into networkx:main Sep 27, 2024
13 of 14 checks passed
@jarrodmillman jarrodmillman added this to the 0.3 milestone Sep 27, 2024
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.

4 participants