Skip to content

Comments

Suffix array#57

Merged
lrvideckis merged 46 commits intomainfrom
suffix_array
Jun 18, 2024
Merged

Suffix array#57
lrvideckis merged 46 commits intomainfrom
suffix_array

Conversation

@lrvideckis
Copy link
Member

might add more functions later; we'll see

Copy link
Member

@cameroncuster cameroncuster left a comment

Choose a reason for hiding this comment

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

so if we are bringing in AC library, why not just use their mod-int?

Comment on lines +64 to +65
pub fn new(s: &[usize], max_val: usize) -> Self {
let sa = suffix_array_manual(
Copy link
Member Author

Choose a reason for hiding this comment

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

IK we previously discussed multiple constructors (one taking string as [T], and the other taking string as [usize] or [char] maybe); but I decided against it:

basically we need to store the string as a member var. So in order to have multiple constructors with different types, we need to templatize the string type; which made it less clean for similar reasons why templatized binary trie was less clean (you need a template type with a ton of traits)

So instead I opted for a single constructor, taking [usize]. If the input is a string, you can just map it to this type.

If the input is an array, you can call the compress function first

Copy link
Member

Choose a reason for hiding this comment

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

sounds good

Copy link
Member

Choose a reason for hiding this comment

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

yeah the compress as a separate helper makes a lot of sense, it really doesn't belong to the SA

@lrvideckis lrvideckis merged commit 6bde4c3 into main Jun 18, 2024
@lrvideckis lrvideckis deleted the suffix_array branch June 18, 2024 04:21
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