Correct C++ update + reorganizing Module bindings

[hschreiber]

For homogeneity, I would do the same for Summand and Box as a next step. Are there some methods you would like to add to the bindings of those two classes, while I am at it? (by default I will at least add __setstate__/__getstate__ for both)

Note that I added the method summands_of_dimension_range to the Module bindings (to use like for s in mod.summands_of_dimension_range(d): ...), which you could use instead of get_module_of_degree if no copy is needed.

And please verify that I didn't forgot to add the latest changes you made to the Module bindings.

Edit: Did the changes for the Summand and Box bindings in this PR in the end as it was quick.

For the changes in the AIDA.ipynb notebook, I just replaced the end of box 15 with:

if len(mma_):
    num_parameters = mma_[0].num_parameters
    mma_.box = [[0]*num_parameters,[1]*num_parameters]
    mma_.box = mp.grids.compute_bounding_box(mma_)

[DavidLapous]

@codex review this PR

[chatgpt-codex-connector]

💡 Codex Review

https://github.com/DavidLapous/multipers/blob/d50dbc275f968120cd0d981a340e2cc88fb83403/multiparameter_module_approximation.py#L778-L780
Skip empty tuple components before degree merge

When tuple inputs mix a valid minpres slicer with an empty slicer that has minpres_degree == -1, the earlier all(...) check still accepts it via len(s) == 0, but this direct merge now passes -1 into the C++ merge(..., int) binding, which rejects negative degrees before seeing that the module is empty. The previous m.get_module_of_degree(-1) path produced an empty module, so these partially empty batches now fail instead of ignoring the empty component.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Validate box arrays before indexing both corners

For public calls such as mod.box = np.zeros((1, n)) or constructing a module from a malformed box array, this helper blindly indexes row 1 after wrapping the tensor. The old setter validated that a box has exactly two corners with matching dimensions; without the check, a one-row array can read past the view/crash and a three-row array is silently accepted with the extra corner ignored.

Useful? React with 👍 / 👎.

[hschreiber]

This should be handled better now in commit c1186f8.

[hschreiber]

💡 Codex Review

https://github.com/DavidLapous/multipers/blob/d50dbc275f968120cd0d981a340e2cc88fb83403/multiparameter_module_approximation.py#L778-L780 Skip empty tuple components before degree merge

When tuple inputs mix a valid minpres slicer with an empty slicer that has minpres_degree == -1, the earlier all(...) check still accepts it via len(s) == 0, but this direct merge now passes -1 into the C++ merge(..., int) binding, which rejects negative degrees before seeing that the module is empty. The previous m.get_module_of_degree(-1) path produced an empty module, so these partially empty batches now fail instead of ignoring the empty component.

I found that the behaviour accepting without reacting a negative dimension argument is prone to errors which are difficult to track when debugging. But it makes indeed loop mechanism less straight forward. @DavidLapous Do you prefer I replace if (degree < 0) throw std::invalid_argument("Cannot merge summands of negative dimension."); with if (degree < 0) return; or keep the responsibility on the user side and replace

for i, m in enumerate(modules):
            mod.merge(
                m, input[i].minpres_degree
            )

with

for i, m in enumerate(modules):
            if input[i].minpres_degree >= 0:
                        mod.merge(
                            m, input[i].minpres_degree
                        )

?

[DavidLapous]

I think it's more a minpres_degree<0 assignement problem, so the "bug-if-any" is earlier in the chain. The output will be correct, so no need to throw in that case I think.

Also, could the lifetime of the box be binded to the life of the Module by transfering the ownership of the box to module?

[hschreiber]

Also, could the lifetime of the box be bind to the life of the Module by transferring the ownership of the box to module?

I guess, if the box is a numpy array, you could theoretically move ownership of the data. But it is a bit unsafe and a box is not that big to copy. Also, by copying the way it is done, you are sure to have something C-ordered and continuous. By taking ownership of the data, I would also need to copy the stride and the shape to be sure it always works. Again, feasible, but I am not sure worth the trouble in this particular case.

[DavidLapous]

the inflation is necessary to handle essential summands in the MMA algo, is there a reason to comment this?

[hschreiber]

Because this inflation had no impact on the MMA algorithm. This box was not used after inflation in the rest of the algorithm before the latest changes from the box being moved outside of Module... So either, a bug was introduced way before and the inflation should have been done before copying into the module, or this line should have been removed before doing the latest changes (you did by updating the submodule commit number before I finished the changes).

[DavidLapous]

I think I want to enforce c_contig to all nanobind interface if I can, but I can deal with that later

[DavidLapous]

There is a api.asarray(...,contiguous=True) IIRC for this

[hschreiber]

If you want, but the Numpy_2d_span can handle non contiguous data (as long as coherently non-contiguous like numpy arrays are). So it is not really necessary here.

[DavidLapous]

I want to keep this function as it's necessary for autodiff (although a reasonable interface is not there yet)

[hschreiber]

The method was added back in the last commit. But as it is used nowhere, I could not test it. Could you give me a small script or notebook I could use to test it in the meantime? Otherwise I could add a small unit test on a 3 x 10 grid or similar to have at least something.

Edit: Added a small test in test_mma.py.