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

Add morpheme_list #65

Merged

Conversation

mh-northlander
Copy link
Collaborator

@mh-northlander mh-northlander commented Oct 5, 2021

#55
Replace Vec<Morpheme> by MorphemeList.

@mh-northlander
Copy link
Collaborator Author

mh-northlander commented Oct 5, 2021

Maybe it's better to let Morpheme have Deref<trait morpheme-list> instead of the reference to concrete MorphemeList type

sudachi/src/analysis/morpheme_list.rs Outdated Show resolved Hide resolved
sudachi/src/analysis/morpheme_list.rs Outdated Show resolved Hide resolved
sudachi/src/analysis/morpheme_list.rs Outdated Show resolved Hide resolved
sudachi/src/analysis/morpheme_list.rs Outdated Show resolved Hide resolved
sudachi/src/analysis/morpheme_list.rs Outdated Show resolved Hide resolved
sudachi/src/analysis/morpheme_list.rs Outdated Show resolved Hide resolved
@eiennohito
Copy link
Collaborator

Also, I didn't understand the comment about Deref.

@mh-northlander
Copy link
Collaborator Author

Also, I didn't understand the comment about Deref.

I'm wondering if we can reuse code in python bind part like:

// rust side
struct Morpheme<T> {
    list: T,
    index: usize,
}
impl <T> Morpheme where T: Deref, <T as Deref>::Target: MorphemeListTrait {
    /// 
}

// python side
impl MorphemeListTrait for PyMorphemeList { /// }

struct PyMorpheme {
    inner: Morpheme<Arc<PyMorphemeList>>
}

@eiennohito
Copy link
Collaborator

I believe that Pyo3 does not allow to expose methods from trait implementations to Python, so that part can't go away.

Also, lazy loading WordInfo will require having mutable references to everything, which is not a good thing.

@mh-northlander
Copy link
Collaborator Author

mh-northlander commented Oct 6, 2021

I believe that Pyo3 does not allow to expose methods from trait implementations to Python, so that part can't go away.

Yes, anyway we need to write code to expose functions.
I leave this idea for later.

Also, lazy loading WordInfo will require having mutable references to everything, which is not a good thing.

I faced problems around this and remove WordInfo field.

@mh-northlander
Copy link
Collaborator Author

I changed the type of MorphemeList.input_text from &str to String.
I guess it's better to have &str from the view point of allocation, but failed to pass the reference to the original input text through analysis.

@eiennohito
Copy link
Collaborator

It's OK to have the String there internally. In the memory reuse case we will swap it with the Utf8InputText buffer.

python/src/morpheme_list.rs Outdated Show resolved Hide resolved
sudachi/src/analysis/morpheme_list.rs Outdated Show resolved Hide resolved
sudachi/src/lib.rs Outdated Show resolved Hide resolved
@mh-northlander mh-northlander marked this pull request as ready for review October 7, 2021 07:41
pub struct PyDictionary {
dictionary: Arc<JapaneseDictionary>,
pub dictionary: Arc<JapaneseDictionary>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

you probably meant pub(super)

@mh-northlander mh-northlander merged commit a725dd3 into WorksApplications:develop Oct 7, 2021
@mh-northlander mh-northlander deleted the morpheme-list branch October 7, 2021 09:25
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