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

Fix incorrect heap size calculation #55

Merged
merged 1 commit into from
Feb 12, 2025
Merged

Conversation

ajalab
Copy link
Owner

@ajalab ajalab commented Feb 11, 2025

This PR fixes the incorrect heap size calculation in the FM-index implementations and suffix arrays, as reported in #54.

@ajalab ajalab requested a review from faassen February 11, 2025 01:57
@ajalab ajalab self-assigned this Feb 11, 2025
ajalab added a commit that referenced this pull request Feb 11, 2025
@@ -120,9 +118,8 @@ where
///
/// Sampled suffix array data is stored in this index.
pub fn size(&self) -> usize {
Copy link
Owner Author

@ajalab ajalab Feb 11, 2025

Choose a reason for hiding this comment

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

Maybe the name of this method size is a bit vague. Should we name it heap_size like vers-vec?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd be happy to see this change!

Copy link
Owner Author

Choose a reason for hiding this comment

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

Filed #58 not to forget.

ajalab added a commit that referenced this pull request Feb 11, 2025
Copy link
Collaborator

@faassen faassen left a comment

Choose a reason for hiding this comment

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

Did I break this code in my porting to vers vec? If so it's good you caught it!

@ajalab ajalab merged commit 68c8357 into master Feb 12, 2025
2 checks passed
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