Skip to content

PyMathics fixups #996

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.

Sign up for GitHub

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

Merged
rocky merged 4 commits into from
Nov 3, 2020
Merged

PyMathics fixups #996

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6775878
Get PyMathics minimally working
rocky Nov 1, 2020
e706650
Remove pymathistestmodule...
rocky Nov 1, 2020
f422f9b
Better parse resolution in the face of PyMathics`
rocky Nov 1, 2020
d8ecaef
PyMathics fns and vars called w/o qualifying
rocky Nov 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 13 additions & 17 deletions mathics/builtin/assignment.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1692,23 +1692,6 @@ class LoadModule(Builtin):
>> LoadModule["sys"]
: Python module sys is not a pymathics module.
= $Failed
# >> LoadModule["pymathics.testpymathicsmodule"]
# = pymathics.testpymathicsmodule
# >> MyPyTestContext`MyPyTestFunction[a]
# = This is a PyMathics output
# >> MyPyTestContext`MyPyTestSymbol
# = 1234
# >> ?? MyPyTestContext`MyPyTestFunction
# =
# . 'MyPyTestFunction'[m]
# . Just an example function in pymathics module.
# .
# . Attributes[MyPyTestContext`MyPyTestFunction] = {HoldFirst, OneIdentity, Protected}
# >> Quit[]
#n>> MyPyTestContext`MyPyTestSymbol
# = MyPyTestContext`MyPyTestSymbol
# >> ?? MyPyTestContext`MyPyTestFunction
# = Null
"""
name = "LoadModule"
messages = {'notfound': 'Python module `1` does not exist.',
Expand All @@ -1727,4 +1710,17 @@ def apply(self, module, evaluation):
except PyMathicsLoadException as e:
evaluation.message(self.get_name(), 'notmathicslib', module)
return Symbol('$Failed')
else:
# Add PyMathics` to $ContextPath so that when user don't
# have to qualify PyMathics variables and functions,
# as the those in teh module just loaded.

# Following the example of $ContextPath in the WL
# reference manual where PackletManager appears first in
# the list, it seems to be preferable to add this PyMathics
# at the beginning.
context_path = evaluation.definitions.get_context_path()
if "PyMathics`" not in context_path:
context_path.insert(0, "PyMathics`")
Copy link
Contributor

@mmatera mmatera Nov 2, 2020

Choose a reason for hiding this comment

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

The context of the symbols in the pymathics module should be pymathics or pymathicsmodulenale` ?

Copy link
Member Author

@rocky rocky Nov 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This is an interesting question for discussion.

A short summary of my thinking right now is PyMathics for now and maybe later PyMathics<tick>module later. But please give me your thoughts.

Let me review history.

In v1.0 natlang was a "buitin" and so its context was I guess System. Do I have that right? I get conflicting reports from mathicsscript --full-form:

In[1]:= Simpify[x 2 - x]
Global`Simpify[System`Plus[System`Times[Global`x, 2], System`Times[-1, Global`x]]]
Out[1]= Simpify[x]
In[2]:= ?Simplify
System`Information[System`Simplify, System`Rule[System`LongForm, System`False]]

I am guessing that first Global is wrong or incomplete

Note that right now, we don't break out "System" functions and variables, although they in fact are in different Python modules/files.

So then we split this off. As best as I can tell, dealing with the context might have gotten messed up or had been incomplete. At least, I had a hard time trying get it to the consistentcy it is now.

The choice here was to give all PyMathics modules a PyMathics context rather than add this to System which I think happened in some (all?) cases.

And in doing that, part of the problem I ran into was with in parser.convert.py which tries to add the context prefix and seems to get it wrong occasionally as seen above with Global

The downside of putting a lot of stuff under a single context is the possibility for name clashes. And that is of course fixed by adding the module name. I guess we could get the name from pymathics_version_data.name.

But in practice, right now there are going to be very few PyMathics modules (that is until Gark writes the definitive guide for writing Mathics builtins).

Probably the only PyMathics modules that are going to exist are the ones we create, which might number as many as 4. There is NatLang and in the future Graph. Possibly the Chemical Data should be pulled out too since that feels a bit specialized, like Natural Language Processing.

(Note: At some point I'll write a pytest for LoadModule which will basically create the Hello PyMathics module inside the test since that code is so short).

So while I think ultimately this is a good thing to do, right now this PR is a bit large.

And there are more things to do just related to this. Renaming test.py to doctest.py and making sure it can be used as a module for testing from PyMathics is an example.

So in sum, my opinion is let's just do this step, let it settle and work out the kinks. And then continue doing more ambitious, more correct, other kinds of things.

Copy link
Contributor

@mmatera mmatera Nov 3, 2020

Choose a reason for hiding this comment

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

As I told you before, I think this PR is a great improvement, and I would merge it as it is. What I wanted to point out that the scope of the modules is something to think about it in the future, and I wanted to have your impression of what would be the best way to handle it.

Copy link
Member Author

@rocky rocky Nov 3, 2020

Choose a reason for hiding this comment

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

Ok -- thanks. We are in agreement then.

As you say, it is something to do for the future.

Feel free to open issues if that's the way you want to go. For myself, there are so many things that I am aware of that should be addressed, issues in general haven't been that helpful to me as it seems for others.

evaluation.definitions.set_context_path(context_path)
return module
Loading