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

Split main #14

Merged
merged 14 commits into from
Feb 23, 2023
Merged

Split main #14

merged 14 commits into from
Feb 23, 2023

Conversation

mmatera
Copy link
Contributor

@mmatera mmatera commented Feb 20, 2023

Just a first round. The documentation was minimally completed. The criteria to split the modules were

  • Following WR guides.
  • each module depends on the minimal number of Python libraries backends.
  • Auxiliary functions moved in separated modules, with a no_doc=True attribute.


summary_text = "retrieve a list of common words"

def eval(self, evaluation: Evaluation, options: dict):
Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Member

@rocky rocky Feb 22, 2023

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

Copy link
Member

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.

@rocky
Copy link
Member

rocky commented Feb 20, 2023

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. \
Copy link
Member

@rocky rocky Feb 22, 2023

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.

Copy link
Contributor Author

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.

@rocky
Copy link
Member

rocky commented Feb 22, 2023

I think we are pretty close.

@rocky rocky merged commit 5128810 into master Feb 23, 2023
@rocky rocky deleted the split_main branch February 23, 2023 23:07
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.

None yet

2 participants