ecboghiu / inflation

Implementations of the Inflation Technique for Causal Inference.
GNU General Public License v3.0
22 stars 3 forks source link

Problems while migrating to Numpy 2 #144

Open apozas opened 4 weeks ago

apozas commented 4 weeks ago

I will use this issue to list the problems I face while attempting to make the code compatible with Numpy 2, in case any of us finds a way to solve them. This can also serve as a list of the major changes introduced, in case we have to debug strange errors afterwards.

In what follows, the comparisons are made both on Python 3.9.16, inflation 2.0.0-devel, and between numpy 1.26.4 (the latest before numpy 2) and numpy 2.0.1

Different behavior of integers with unspecified type (Status: open)

See the following two snippets:

For numpy 1

Python 3.9.16 (main, May 15 2023, 23:46:34) 
[GCC 11.2.0] :: Anaconda, Inc. on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import numpy as np
>>> a = np.array([0], dtype=np.uint8)
>>> a[0]-1
-1

Moreover, the type of the output is int64.

For numpy 2:

Python 3.9.16 (main, May 15 2023, 23:46:34) 
[GCC 11.2.0] :: Anaconda, Inc. on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import numpy as np
>>> a = np.array([0], dtype=np.uint8)
>>> a[0]-1
<stdin>:1: RuntimeWarning: overflow encountered in scalar subtract
np.uint8(255)

The numpy 2 behavior makes sense: if the integer is of undefined type, numpy assigns the type of the scalar it is operating with and performs the operation, producing a result whose type matches that of the only one specified. However, that breaks our code. In particular, it is producing an overflow in InflationProblem-L850 when assigning the party index to the zero monomial, since the zero monomial is initialized in an internal data type that is computed in the code but seems to eventually be uint8. This, for instance, breaks test_sanitize.

I propose as a solution substitute the code that computes the internal data type by just np.int64. If, for reasons of space and efficiency, we wanted to be more fine-grained because we may expect different types for different problems, I would then suggest to analyze _np_dtype and, if it is of an unsigned character, substitute it by its corresponding signed version.

This seems to be the only problem that I have encountered. In fact, just changing _np_dtype to int64 makes all the tests pass. And, moreover, int8 seems to suffice to pass all the tests. There are a few additional deprecation warnings and so on, but nothing too complicated.

EDIT 30/AUG/24: After the problem was seemingly solved by https://github.com/ecboghiu/inflation/commit/63f8ed0dfa0546c0433a5cd2975bc39fa87e76b1, I found similar errors when discovering normalization constraints. The problem is caused by the type of lexorder, so an initial solution has been to change the type only of lexorder from uint8 to int8 (f3b01909b05cfd2a39dfb75bd8757a9212f3209d). This allows us only to consider cardinalities smaller than 128, but this seems an a priori reasonable maximum number of parties, copies, inputs or outputs.

Replacement of deprecated functions (status: finished)

After removing the DeprecationWarning filter in the tests I saw that there was only one function that needed amend: np.in1d had to be changed to np.isin. This is done in 532284cc9e828fb1cd15638fa6acbc02459e05e4.

eliewolfe commented 3 weeks ago

I disagree about switching to np.int64 everywhere. That is a huge waste of memory. When we use np.uint8, already the LP's can get memory expensive. The best way to fix this is to to avoid creating a "rectified" ndarray for the zero monomial. (Which we probably should have done anyway, since it returns zero.) I'll try to make a fix. An alternative fix is to add "-1" to the sorts of integers we want the internal data type to be able to represent. That would, in practice, mean using np.int8 instead of np.uint8. No additional memory cost, but we would not only be able to represent integers up to 127 instead of 255. (Which is probably fine.)

apozas commented 3 weeks ago

Then, I think it is worth spending a bit of time trying to avoid creating the rectified zero monomial, if possible. If that fails, we have the option of moving to np.int8. But before doing this second option, I will look at the commit history. I remember that at some point (long time ago) I faced an overflow in a not-that-large scenario and we fixed it by modifying some internal data type.

eliewolfe commented 2 weeks ago

Fixed in commit 63f8ed0dfa0546c0433a5cd2975bc39fa87e76b1. @apozas confirm and close issue?

apozas commented 2 weeks ago

Seems like it's working, yes. All tests pass, and the first examples in the Examples notebook produce the exact same output. Let me finish up some small checks in documentation and I will close the issue in the next few days,

apozas commented 2 weeks ago

Somehow I re-ran the tests and I had a few failing. The reason is that lexorder is by default of type uint8, but when computing normalization equations we are subtracting 1 to the party index, which can be 0.

I have fixed this in https://github.com/ecboghiu/inflation/commit/f3b01909b05cfd2a39dfb75bd8757a9212f3209d by changing the type only of lexorder from uint8 to int8. This allows us only to consider cardinalities smaller than 128, but this seems an a priori reasonable maximum number of parties, copies, inputs or outputs. I am however happy to accept a different solution if you feel it is better.