Numeric array as atom take 2#9
Conversation
| if isinstance(expr, Atom): | ||
| if isinstance(expr, ByteArray): | ||
| return ListExpression(*expr.items) | ||
| if hasattr(expr, "items"): |
There was a problem hiding this comment.
Generalized this to work with any atom that defines .items
| if not hasattr(expr, "items"): | ||
| evaluation.message("First", "normal", Integer1, expression) | ||
| return | ||
| expr_len = len(expr.items) |
There was a problem hiding this comment.
Generalized this to work with any Atom that defines .items.
Suggestion: to simplify some of this code, add to Expression a method that returns .elements, .items, or None depending on which are defined. Maybe call it get_parts() as I think it is precisely what the Parts builtin operates on.
Note that I did not make this generalization in Last, as it is awaiting DRYing.
Suggestion: generalize eval_Part in a similar way (operate on .items or .elements by calling get_parts as I suggested above) and DRY both First and Last to just call eval_Part(..., 0) and eval_Part(..., -1) respectively.
There was a problem hiding this comment.
I'd like to mull this over. My personal trepidation using the name "part" is that Part in Mathematica more closely corresponds to a Python or Golang slice as best I can tell.
But is that what's meant here?
FYI the common DRY'd part of First and Last would probably live under mathics.eval, not mathics.builtin. The current philosopy is to only do signature matching and argument checking in mathics.builtin. while mathics.core has the function part. By doing this, internal routines can call internal routines without having to go through the tedious and slow lookup process. Also, an instruction interpreter doesn't do OO, it is purely a function call kind of thing.
There was a problem hiding this comment.
For List, NumericArray, and ByteArray, I think they are essentially slices, and I think they are exactly what Part with a single index will return. Maybe there are other kinds of things that have .elements or .items where this is not true? Maybe we can discuss more on Tuesday after you've had a chance to mull it over.
There was a problem hiding this comment.
Given how general Mathematica Part is compared to Python's and GoLang's slices, maybe "slice" is a better name since it better matches the more limited here?
Personally, I like "slice" for this concept over "Part". There is something about Mathematica where the terms used while not wrong, can tend not to be the conventional terms. But if something strongly matches Mathematica, I have been advocating using the Mathematica term.
There was a problem hiding this comment.
They also aren't slices in the full generality of what you can do with slices in Python* either. While it's true that Part can return more kinds of things, it still seems natural to me to call them parts because they are a kind of thing that can be returned by Part. But ultimately from my perspective it's your call.
* Python slices for lists have start,stop,step. And for numpy arrays you can do slicing along any and all axes, e.g. for a three dimensional array you can do a[0, 3:5, [1,2,7]] which picks out index 0 on axis 0, indexes 3,4,5 on axis 1, and indexes 1,2,7 on axis 2. This works because Python just passes whatever is between the [] to __getitem__. It gets even funkier - you aren't limited to passing lists like [1,2,7] in, you can pass numpy arrays containing indexes; I think this inserts extra dimensions at that spot in the shape, iirc.
There was a problem hiding this comment.
Thanks for the information. You've convinced me.
(Golang's slices are very restrictive; but that's golang.)
| } | ||
|
|
||
|
|
||
| # |
There was a problem hiding this comment.
I propose to split this off (together with a couple minor changes in other files, but not the NumericArray builtin, Part, First, Normal, etc.) into a separate PR - what do you think?
There was a problem hiding this comment.
Deferring Part, First, Normal, etc. is fine. As you've seen, it's okay to defer implementation when it's made explicit what's needs to be done (and that in of itself will be long and involved).
I had in the back of my mind to split out the mathics.atom into mathics.atom.numeric, mathics.atom.array, mathics.atom.symbol, and mathics.atom.string (or something like that). But since that will be another revision here, I suppose I can defer until after NumericArray part 1.
There was a problem hiding this comment.
OK. So here's what I think should go in Part 1 from this PR:
mathics/core/atoms.py
mathics/core/convert/python.py
mathics/core/keybomparable.py
the first section of test/builtin/test_numericarray.py, which relates to NumericArray atom as Python object
How would you like to proceed?
- I prepare a PR with those changes and submit upstream and we all review there?
- I prepare a PR with those changes and submit against my fork (like this PR) for you to review first?
- You first review just those files in this PR then we can submit as PR upstream?
- You mull it over for a while and we discuss on Tuesday?
There was a problem hiding this comment.
OK. So here's what I think should go in Part 1 from this PR: mathics/core/atoms.py mathics/core/convert/python.py mathics/core/keybomparable.py the first section of test/builtin/test_numericarray.py, which relates to NumericArray atom as Python object
Great!
How would you like to proceed?
- I prepare a PR with those
hangesand submit upstream and we all review there?
Perfectly fine.
- I prepare a PR with those changes and submit against my fork (like this PR) for you to review first?
Up to you.
- You first review just those files in this PR then we can submit as PR upstream?
Up to you.
- You mull it over for a while and we discuss on Tuesday?
Will probably do in any event.
| @@ -113,5 +118,7 @@ def from_python(arg: Any) -> BaseElement: | |||
| from mathics.builtin.binary.bytearray import ByteArray | |||
|
|
|||
| return Expression(SymbolByteArray, ByteArray(arg)) | |||
There was a problem hiding this comment.
@rocky Shouldn't this just return ByteArray(arg)?
|
One thing I'd ask. Maybe open an issue describing the overall thing. In that there can be a checklist of items to do. This would include stuff in the PR part 1 as well as remaining items. When the PR is done, we will still have a record of the remaining items. Or two issues could be filed of part 1 or and remaining. Your choice. The important part though is to have an record in the issues of what is needed (to compare against what was done). |
|
Perfectly sensible, will do. On Nov 9, 2025, at 16:50, R. Bernstein ***@***.***> wrote:rocky left a comment (bdlucas1/mathics-core#9)
One thing I'd ask. Maybe open an issue describing the overall thing. In that there can be a checklist of items to do. This would include stuff in the PR part 1 as well as remaining items.
When the PR is done, we will still have a record of the remaining items. Or two issues could be filed of part 1 or and remaining. Your choice.
The important part though is to have an record in the issues of what was needed and what was done.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>
|
|
2 participants
@rocky new version of PR after updating to latest master. (I had intended to update the previous PR, but I did something wrong and all the recent updates to upstream master got included in the PR, so I just started a new PR).
In this PR I have made some changes to make Normal and First work, but there's more to be done there and that's not my main concern - not needed for the plotting stuff. So in the interest of merging early, I'd propose to split this into two pieces:
1 - NumericArray atom per se. This is I think nearly there, it is sufficient for my work on plotting, and having it in master will help
2 - NumericArray builtin, Normal, First, Part, etc. I don't need these from my perspective I'd like to decouple this work from my work on plotting.
I'll leave some comments here on the things I did for Normal, First, Part etc. for your consideration.