[tias]

follow numpy's current recommendations for subclassing ndarray (our NDVarArray which should have been called NDExprArray):

• introduce array_finalyze instead of init
• introduce array_ufunc (and _elementwise) instead of _vectorized and the overloaded ops

[tias]

as an alternative to #625

it was actually a test of the new Cursor Composer Grok 4.5, and the process as well as the result is surprisingly simple enough that I wanted to share it.

I am NOT saying this should be in 1.0.0, because it is rather very late and drastic.

[tias]

@IgnaceBleukx you add 'out' to array ufunc, and also modified the doc of sum/prod/max/min/all. So these do not fall under array_ufunc? or sum/prod maybe does and I should have deleted it?

[IgnaceBleukx]

Ah I didn't mean to fully wipe out the docstring on array ufunc... the out-bit should just have been an addition to the original docstring; I'll fix it.

For the docs on sum/prod etc, these are on the NDVarArray.sum functions, right? So its a different path compared to the ufuncs, which do element-wise operations. Here its really about summing the whole array. We could actually also support a bit more kwargs there like where, out etc. This was an attempt to match the signature of ndarray.sum but its not complete either...
https://numpy.org/doc/2.2/reference/generated/numpy.ndarray.sum.html

[Dimosts]

I really like it, it is so clean! And actually saves code and uncomplicates it.

Only a couple of non-blocking things:

• This does not only make it cleaner, but change behaviour compared to what we describe in the docs, in modeling.md (e.g. "result always keeps the shape of the CPMpy array”). Docs should change in this PR I believe.
• Supporting x +=1, it means we change the variable array x we created before, that could be confusing for the user (and I am not so sure we should support it)
• There seems to be an issue with _has_subexpr in some local edge case tests. out= does clear it on destination, but if the destination is a view then this could be a problem. Simple case: grid = intvar(1, 9, shape=(3, 3), name="g"); grid[0, :] += 1. grid.has_subexpr() would stay False. Should we only cash true?
• the value() fix is nice, and highlights indeed that it is actually an array of expressions. This should also be reflected in the docstrings (e.g. in value() it says "stored variables"), but also in clear()? `clear()`` now crashes if not everything is a variable.

[tias]

@IgnaceBleukx last commit, did you mean isisntance(NumVarImpl)?

[IgnaceBleukx]

Right, that was another general topic I wanted to discuss... what should be the behaviour of NDVarArray.clear when one of the elements is an expression? The value of an expression is determined by the values of its arguments... so either we raise an error that this is ill-defined, or we do a get_variables(elem_that_is_an_expression) and clear the values of those variables?

Personally I would opt for raising a ValueError

[IgnaceBleukx]

I've removed the support for out= again, as it is indeed quite confusing...
Cleaned up the .value and .clear methods as well, and added some tests.

For docs: I just removed the warning about the inconsistencies between numpy and cpmpy, or do we want to add something more there? I'm not sure

[Dimosts]

I think it is ready for merge.

Only 2 non-blocking, minor, things:

• First is on docs, meaning it can also come later. in modeling.md we say "This is not guaranteed for all NumPy functions though — np.equal(x, y) for example returns a plain Boolean array, not constraints.", which is not a good example anymore.
• Second is again for clear(). Personally I would prefer that as value() handles expressions, clear() should also. But if we want to handle only vars, and throw valueerror otherwise, we could do it cleaner, i.e., at least clear() all vars before we throw the valueerror, or no vars. Currently it clears all vars until it encounters any non-var element, and leaves the rest uncleared. I cannot think of an actual use-case that a user would catch the valueerror and want to handle it, but in an edge case this could be an issue. It is too much of an edge case though, so I am fine with also merging it as is.

Overall, approved.

[IgnaceBleukx]

I agree on the docs, not sure about the clear stuff.
As you said, there is no real obvious reason why a user would like to catch the ValueError and then continue some program... I'm ok keeping it as is for now : )