8000 refactor: removed lazy_static! dependency and cleaned up scores by abstractqqq · Pull Request #639 · rust-bio/rust-bio · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

refactor: removed lazy_static! dependency and cleaned up scores #639

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

refactor: removed lazy_static! dependency and cleaned up scores #639

wants to merge 6 commits into from

Conversation

abstractqqq
Copy link
@abstractqqq abstractqqq commented May 14, 2025 •
edited
Loading

hi, I am reviewing the code and I think we should prefer a simpler approach to a few things.

  1. There is no need to save the PAM/BLOSUM matrices as an Ndarray matrices, and use lazy_static!. We can simply use a const fixed-sized array for that.
  2. We can reuse the same code for lookup in all scores.
  3. Other uses of lazy_static! can be again replaced by pre-computed arrays with a comment that explains how to generate them.
  4. Using lazy_static will initialize the data upon first use. Sure, it might save some memory if they are not used, but overall we are saving very little (a few kilobytes at most).
  5. Coming from the Python world, I would like to keep dependencies to a minimum and since there is no hard-need on lazy_static!, I removed it and directly put data in code, which imo is simpler and more straightforward.

johanneskoester
johanneskoester requested changes May 14, 2025
View reviewed changes
Copy link
Contributor
@johanneskoester johanneskoester left a comment

Choose a reason for hiding this comment

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

Thanks for the work! However, I think the maintainabiliy of lazy_static is an advantage here that is more important than the additional dependency. However, I would be fine with a build-time based approach as a replacement for the lazy-static.

@fxwiegand
Copy link
Contributor

Probably worth pointing to the lazy static deprecation issue as a reference. Seems like lots of people lean to migrating to once_cell.

@tedil
Copy link
Member
tedil commented May 15, 2025

Probably worth pointing to the lazy static deprecation issue as a reference. Seems like lots of people lean to migrating to once_cell.

Or even use LazyLock from std! Available since 1.80

@abstractqqq
Copy link
Author
abstractqqq commented May 16, 2025 •
edited
Loading

Probably worth pointing to the lazy static deprecation issue as a reference. Seems like lots of people lean to migrating to once_cell.

Or even use LazyLock from std! Available since 1.80

Following Tedil's advice, I have replaced where we used to use lazy_static with LazyLock.

I still think using a Array2 in BLOSUM matrix is overkill. Using a fixed sized array and very simple index math is enough.

@fxwiegand
Copy link
Contributor

Following Tedil's advice, I have replaced where we used to use lazy_static with LazyLock.

This of course increases the MSRV. That needs to be updated in the CI file.

@abstractqqq
Copy link
Author

I wonder how sensitive is this.. Maybe there is no point in making this update if it means we have to force users to use a higher version which might break things.

@fxwiegand
Copy link
Contributor

I wonder how sensitive is this.. Maybe there is no point in making this update if it means we have to force users to use a higher version which might break things.

I don't feel like this would be an issue. Would be happy to get @tedil opinion on this though!

@fxwiegand fxwiegand requested a review from johanneskoester June 4, 2025 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johanneskoester johanneskoester Awaiting requested review from johanneskoester

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@abstractqqq @fxwiegand @tedil @johanneskoester
0