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

Remove unused imports #35

Merged
merged 2 commits into from
Aug 3, 2023
Merged

Conversation

EliahKagan
Copy link
Collaborator

@EliahKagan EliahKagan commented Aug 1, 2023

This removes unused imports, both in the current code (npc_gzip/, examples/, and tests/) and in the old code (original_codebase/).

Either all or nearly all unused imports are removed, depending on precisely how one defines "unused." I avoided removing imports (or making any changes) where imports are unreferenced but in some other sense used, such that removing them without making other code changes would cause breakage or alter behavior.

Info on "unused" imports that are needed and not removed (click to expand if interested)

There are only two such places.

  • In the new code, npc_gzip/__init__.py, has imports but no __all__, so technically those imports are nonpublic, and since no code in that file uses them (there is no other code in that file), they are "unused". It may be better to avoid * imports and assign __all__ in that file, but I would consider such a change outside the scope of this PR.
  • In the old code, eval(args.dataset)(root=args.data_dir) is used to load datasets dynamically in a way that requires them to have been imported into the module, yet the imports are "unused" in the sense that no code written in the module refers to them. It may be best to have a comment about this, or use importlib instead of an eval-based way, but such changes are not necessarily justified for the original codebase that is not under active development. Even if such changes are made, they, too, are outside the intended scope of this PR.

All other unused imports are removed.

Although major changes probably shouldn't be made to the original codebase without strong reason, it is clear from context that the unused imports in it are mostly due to other fairly recent changes (some as recent as #30).

I have tested this change, both in the new code by running pytest in the poetry-managed environment (in Python 3.11, but also CI checks this for 3.9, 3.10, and 3.11), and in the original codebase by installing its dependencies in a Python 3.10 virtual environment and running python main_text.py and python main_text.py --para.

This does not remove any imports in npc_gzip/__init__.py, which are
conceptually used.

All other unused imports in modules in the npc_gzip, examples, and
tests directories are removed.
This removes all unused imports in modules in the original_codebase
directory, except for the dataset imports in main_text.py, which
are not referenced in the code but have to be imported, due to the
way they are loaded dynamically based how the script is invoked.
@EliahKagan EliahKagan marked this pull request as ready for review August 1, 2023 23:19
@bazingagin bazingagin merged commit 9345ec8 into bazingagin:main Aug 3, 2023
@EliahKagan EliahKagan deleted the unused-imports branch August 3, 2023 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants