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

Cleaning up dead code #156

Merged
merged 13 commits into from
Oct 1, 2024
Merged

Cleaning up dead code #156

merged 13 commits into from
Oct 1, 2024

Conversation

Theodlz
Copy link
Collaborator

@Theodlz Theodlz commented Sep 19, 2024

Jokes aside, this PR does a few things:

  • remove a lot of unused imports
  • remove some unused methods
  • introduce numba to speed-up methods that can be speed up as is (some refactor could allow us to use it in many more functions
  • lint some files
  • remove unused arguments from some methods
  • remove a bunch of unused variables within methods

The reviewer should carefully test this, but the changes should be very minimal

@Theodlz Theodlz requested a review from mcoughlin September 19, 2024 11:58
@Theodlz
Copy link
Collaborator Author

Theodlz commented Sep 19, 2024

@mcoughlin any idea why the CI doesn't run by default?

@Theodlz
Copy link
Collaborator Author

Theodlz commented Sep 19, 2024

btw might want to retire 3.8 testing in favor of 3.12, or just add 3.12

@Theodlz Theodlz self-assigned this Sep 19, 2024
@Theodlz
Copy link
Collaborator Author

Theodlz commented Sep 19, 2024

tests not working locally either (incorrect imports of MOCpy in a few places). Fixing it rn

@Theodlz
Copy link
Collaborator Author

Theodlz commented Sep 19, 2024

I found myself having to remove a lot of stuff that did seem relevant in gwemopt.io.read_skymap(), because it was creating this moc_keeps variable to pass to mocpy's MOC.probabilities_in_multiordermap() in gwemopt.tiles.compute_tiles_map(), except that MOCpy doesn't take that as an argument anymore?

A little odd, not sure what it does

@mcoughlin
Copy link
Collaborator

moc_keeps was originally intended to track which pixels we should always observe, even in case of incrementing the telescopes where we observe and then remove a region. However, I don't think we really use it, so it could be safely ripped out.

@Theodlz
Copy link
Collaborator Author

Theodlz commented Sep 19, 2024

@mcoughlin any idea why the CI doesn't run by default?

let me know if you've experienced that before, no idea how to fix it

@Theodlz
Copy link
Collaborator Author

Theodlz commented Sep 19, 2024

ok some unexistent CI runs where set in the branch rules. Removed that but still not running. Looks like there is a schedule set in the CI, maybe that's why? Like it only runs it at a specific time so these can't run?

@mcoughlin
Copy link
Collaborator

Idk I would have thought I copied this from skyportal but @robertdstein had tried to improve this.

@Theodlz
Copy link
Collaborator Author

Theodlz commented Sep 19, 2024

ah... github had turned off the CI on this repo for lack of activity... except it's not really clearly flagged...

@Theodlz Theodlz changed the title Slowly but surely deleting gwemopt Cleaning up dead code Sep 19, 2024
@Theodlz
Copy link
Collaborator Author

Theodlz commented Sep 22, 2024

Had to resolve some conflicts with main @mcoughlin, and some of them deleted some code which I didn't, so might want to look at my latest commit to double check that. Otherwise, this should be mergeable once green :)

@mcoughlin
Copy link
Collaborator

@Theodlz sorry a few more conflicts maybe.

@Theodlz
Copy link
Collaborator Author

Theodlz commented Oct 1, 2024

@mcoughlin should be all good now

Copy link
Collaborator

@mcoughlin mcoughlin left a comment

Choose a reason for hiding this comment

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

Thanks @Theodlz

@mcoughlin mcoughlin merged commit 8fa5ce0 into skyportal:main Oct 1, 2024
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants