-
Notifications
You must be signed in to change notification settings - Fork 1
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
Split main #14
Split main #14
Conversation
|
||
summary_text = "retrieve a list of common words" | ||
|
||
def eval(self, evaluation: Evaluation, options: dict): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this function, I am seeing
RecursionError: maximum recursion depth exceeded while calling a Python object
Maybe we comment it out for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my machine and in the CI it seems to work. Do you have a test case which raise this error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you try the first Form WordList[]
which is what I believe this particular eval
method handles? There is no CI test for this, so unless you ran this explicitly you might not know it fails.
Also, a principle mentioned in the guide says that every builtin should have at least one example so that might lead you to a problem as well if there is one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried with WordList[]//Length
. Now I added this as a doctest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And yes, the CI is still clean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried with
WordList[]//Length
. Now I added this as a doctest.
Ok - good. Details are important, giving these up front saves time.
Yes, WordList[]//Length
works for me as well. But WordList[]
by itself does not and gives me the recursion error.
This is something we should point out.
Also:
Length[WordList[]] > 10000
is not not something that a user is going to be interested in. This feels like something that belongs as a pytest instead. (The distinction between user example and doctest was also mentioned in the guide.)
See the section under Applications in https://wolfram.com/xid/0cq048d2-ov2unu for something that is more akin to a user-oriented example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WordList[];
does not produce the issue either. So, it seems to be a problem in showing large lists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WordList[]
also works in my system, but takes time to print the result.
from mathics.core.list import ListExpression | ||
|
||
from pymathics.natlang.spacy import _cases, _pos_tags, _position, _SpacyBuiltin | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a Python statement like:
sort_order = "Text normalization"
Currently in the summary list we have:
- Linguistic Data
- Text normalization
- Text analysis functions
- Language translation
"Text Normalization" appears before "Language translation" because of the order of the modules: "normalization" appears before "translation".
Similarly for the other modules.
Over all, I like this, very much. This is a big improvement over what came before! Thanks for undertaking this. |
|
||
## Problem with import for certain characters in the text. | ||
## >> text = Import["ExampleData/EinsteinSzilLetter.txt"]; | ||
>> text = "I have a dairy cow, it's not just any cow. \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrapping does not render right for "\" in test cases. Look at the Dango rendering here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a pity. Now I removed the extra spaces just by eliminating the spaces from the left indent. I also noticed that in Django, linebreaks are ignored when strings are shown.
I think we are pretty close. |
Just a first round. The documentation was minimally completed. The criteria to split the modules were
no_doc=True
attribute.