Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Efficient imports #5024

Merged

Conversation

GarethCabournDavies
Copy link
Contributor

@GarethCabournDavies GarethCabournDavies commented Jan 24, 2025

In an attempt to remove "from xxx import *"-style imports from more places, I've used git grep import | grep "*" | grep -v css| grep -v __init__ to find any places where this is used.

I then commented out the import, ran the tests to find what needs importing and getting that explicitly (not the most sophisticated, but seems to have worked).

After this change there is now 1 place which uses "from xxx import *", which is the versioning module, and I think that is valid.

The changes are mainly in the test suite - I've tried as much as possible to get tests to now pass, using cpu and cuda schemes, as these were what was catered for before!

I'd like to have other schemes / backends working, and some cuda tests fail, but that is for another PR

Standard information about the request

This is a code quality change
This change affects: the offline search, the live search, inference, PyGRB

This change: has appropriate unit tests, follows style guidelines (See e.g. PEP8), has been proposed using the contribution guidelines

Motivation

#4783

Testing performed

Ran tests manually - they worked. The CI will also check this

  • The author of this pull request confirms they will adhere to the code of conduct

@GarethCabournDavies GarethCabournDavies added the code-quality Improvements to code quality label Jan 24, 2025
Copy link
Contributor

@titodalcanton titodalcanton left a comment

Choose a reason for hiding this comment

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

This touches only tests and banksim/faithsim engines, so I think it is ok to proceed. Just noting that it is not immediately obvious to me what is going to happen when changing the schemes like that in some of the tests, though I suspect that has no effect.

@GarethCabournDavies GarethCabournDavies merged commit 178a742 into gwastro:master Jan 28, 2025
30 of 31 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
code-quality Improvements to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants