Skip to content

Fix part assign shared: resolve merge conflict - #2095

Closed
rocky wants to merge 15 commits into
masterfrom
fix-part-assign-shared-resolve-merge-conflict
Closed

rocky wants to merge 15 commits into
masterfrom
fix-part-assign-shared-resolve-merge-conflict

Conversation

@rocky

@rocky rocky commented Oct 31, 2026

Copy link
Copy Markdown
Member

Resolve the merge conflict by accepting new tests that have been added to master.
Perform some Ruff and mypy fixes that are not in a merge to a follow-on PR that has mathics.core.list ruff and mypy changes.

tombouctou and others added 15 commits October 29, 2026 19:04
x[[{2, 5}]] = v crashed (SubExpression returned None for a
ListExpression position), x[[All, 1]] = v assigned to x[[1]] instead of
the first element of every row, x[[-1]] = v wrote the wrong element, and
a part that does not exist raised IndexError after some parts had
already been changed.

Positions are now resolved and checked before anything is replaced:
a list of positions, All and a Span select several parts, negative
positions count from the end, and a missing part gives Part::partw and
leaves the value unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
l[[0]] = f with l a List, m[[2, 0]] = g on a nested List, and k[[0]] =
List crashed with a TypeError: a List cannot take another head in
place. The part is now replaced by a new expression with the new head.
The value assigned to is held while it changes, so that the value itself
can be replaced as well.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
b = a; b[[2, 1]] = 9 changed a too, and k[] := (w = {1, {2}};
w[[2, 1]] = 5; w) changed the definition of k: the parts on the way to
the assigned position were changed in place, though they may be shared
with other values and definitions. They are now copied first.

A ListExpression changed with set_element works out again whether it
is literal. Span positions counted from the end, and spans with a
negative step, now select the parts WMA selects.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
pa[[{2, f[x]}]] = 5 raised an AttributeError; it now gives the message
WMA gives and leaves pa unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t again

Each p[[i]] = v on a long list went over all its elements several
times: set_element worked out again whether the list is literal, and
afterwards its elements properties and its symbol cache were rebuilt,
and the list was evaluated element by element, because they had been
cleared. ListExpression.set_element now updates the literal value, the
elements properties and the cache from the old ones and the new element;
copies of a list keep them too.

A Fisher-Yates shuffle of Range[1000] by Part assignments (Combinatorica's
RandomPermutation) took 11.7 s with the copy on write of this branch and
9.3 s on master; it now takes 2.8 s.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
part[[-4]] = 0 on a list of three elements turned -4 into position 0
and replaced the head, giving 0[1, 2, 3]. A negative position now has
to be between -Length and -1, as in WMA; position 0 is reached only as
0.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@rocky
rocky force-pushed the fix-part-assign-shared-resolve-merge-conflict branch from 205762a to 3d23609 Compare October 31, 2026 09:50
@rocky
rocky requested a review from tombouctou October 31, 2026 09:56
@rocky rocky changed the title Fix part assign shared resolve merge conflict Fix part assign shared: resolve merge conflict Oct 31, 2026
Base automatically changed from fix-part-assign-shared to master October 31, 2026 14:03
@rocky

rocky commented Oct 31, 2026

Copy link
Copy Markdown
Member Author

Merged into branch directly.

@rocky rocky closed this Oct 31, 2026
@tombouctou

Copy link
Copy Markdown
Collaborator

I pushed two commits to this branch after a review. Each commit message has the details.

ecbbd9e28: merge of master

master already has #2084, which makes the same set_element/_copy change, so I resolved the conflicts in mathics/core/list.py and mathics/core/subexpression.py by taking master's version. That removes:

  • _is_atom and _part_positions defined twice (the second pair silently replaced the first; Ruff F811);
  • two mypy errors in list.py: list(self.value) without the or () guard, and element.value on a BaseElement;
  • the Optional/Tuple imports and # -*- coding: utf-8 -*- headers that master had removed.

The # noqa: ISC004 in test_eol.py is kept.

a264d2ba2: spans checked as in WMA; a shared path is copied once

Spans in Part assignment (also wrong on master):

  • A span reaching before the start of the list replaced the head: pa = Range[5]; pa[[-10;;-1]] = 0 gave 0[0, 0, 0, 0, 0], and pa[[-6;;2]] = x gave x[x, x, 3, 4, 5]. A span outside the list now gives Part::take and leaves the value unchanged.
  • An end that is left out or given as All is -1 whatever the step, so pa[[;; ;; -1]] = ... and pa[[3;;All;;-1]] = 0 now give take instead of silently doing nothing.
  • A span may select nothing only right next to the elements: 3;;2, 6;;5, 1;;-6 and 5;;6;;-1 are allowed; 7;;6, -7;;-7, 0;;2 and 4;;2 are errors.
  • About 30 cases were checked against WMA 15 (wolframscript), and most of them are now tests in test_eol.py. One existing test changed: pa[[2 ;; 5]] = 0 on a list of 3 now expects WMA's take message instead of partw.
  • WMA reports these as Set::take, but here they come out as Part::take, the same way partw already does. That is left for a separate change.

Copying:

  • An expression on the way to several parts was copied again for each part. One assignment now remembers the copies it made, by id, keeping the objects alive so an id cannot be reused. m[[1, All]] = 1 on a 300×2000 matrix went from 0.17 s to 0.07 s. A test checks that the shared original is left untouched.
  • _copy of a List now calls shallow_copy() instead of repeating its code. Other expressions still use Expression(head, *elements): Expression.set_element drops the copy's properties and cache straight away, so shallow_copy, which rebuilds the cache, would only add work.
  • The new annotations make mypy check replace, so _headp/_elementsp are now declared and current_index is narrowed.

Left as is on purpose: ListExpression.set_element keeps the old element's symbols in the cache, as decided in #2084. Rescanning the list on every assignment would cost more than the occasional extra re-evaluation.

The list, assignment, core and evaluation tests pass locally (709 passed, 6 xfailed). The full pytest and doctest run is still going.

🤖 Generated with Claude Code

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