lschoe / mpyc

MPyC: Multiparty Computation in Python
MIT License
367 stars 76 forks source link

Fix the inconsistent integral property in np_update #70

Closed MarcT0K closed 12 months ago

MarcT0K commented 1 year ago

This PR aims to fix the behaviour of np_update on SecureFixedPointArray. As discussed in #67 , the integral property was "lost" during the function.

We may need to iterate on the current PR to agree on several points:

lschoe commented 1 year ago

The intended usage of this function is to write things like a = mpc.np_update(a, (0, 1), 42). (We would rather write a[0, 1] = 42 but that is hard to implement as an MPyC coroutine.) In any case, the intended effect is that a is updated in-place. The returned array by mpc.np_update(a, (0, 1), 42) differs on the outside, but the finite fields array inside is updated in-place.

The integral attribute does not need to be modified as you currently do, as it is set for the returned array by the call to returnType().

Regarding the cases for value. That can be assumed to have an integral attribute for now, but that will not always be the case. (Later integer or float, or even a Numpy array with such numbers can be allowed.) Some cases for mismatching values are caught because the finite fields do not match, but let's not go to much into this, because this is also true for lots of other functions.

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (32a6db0) 93.04% compared to head (ad94bc2) 93.05%.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #70 +/- ## ======================================= Coverage 93.04% 93.05% ======================================= Files 16 16 Lines 8902 8907 +5 ======================================= + Hits 8283 8288 +5 Misses 619 619 ``` | [Files Changed](https://app.codecov.io/gh/lschoe/mpyc/pull/70?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Berry+Schoenmakers) | Coverage Δ | | |---|---|---| | [mpyc/runtime.py](https://app.codecov.io/gh/lschoe/mpyc/pull/70?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Berry+Schoenmakers#diff-bXB5Yy9ydW50aW1lLnB5) | `91.82% <100.00%> (+0.01%)` | :arrow_up: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

MarcT0K commented 1 year ago

I updated the PR based on your feedback:

MarcT0K commented 12 months ago

Is the PR all good now?

MarcT0K commented 12 months ago

I've just rebased the branch to solve the merge conflicts

lschoe commented 12 months ago

OK for now, handling the basic case that value has the integral attribute.