Features/681 balanced

[ClaudiaComito]

Description

Issue/s resolved: #681 (see discussion in #668 )

Changes proposed:

• New dndarray property balanced, can beTrue or False, or None if not enough info.

• balanced is set internally within the factories and cannot be specified by the user. It is always set to True except by ht.array where is_split is not None, in which case the balanced status will be assessed (adds one more communication step to ht.array(), but eliminates the need for possibly repeated is_balanced() checks later).

• the dndarray method is_balanced now returns self.balanced, unless self.balanced is None, in which case it will perform the check as usual and set the balanced property.

• One major problem is our extended use of the construct x.larray = new_local_torch_x in code and tests. Very often new_local_torch_x.shape != x.larray.shape and results into a mess, among other things invalidating the balanced information. I've introduced the following changes:

  • added a "local shape sanitation" to dndarray.larray.setter. To me it's still too lax. If not called from all processes, it will produce an imbalanced dndarray with inhomogeneous balanced status.
  • replaced instances of brute-force larray-setting with ht.array(new_local_torch_x, is_split=...) (notably concatenate, argmin and argmax)
  • while I was at it, modified __reduce_op to return the correct argmax/ argmin result directly

• I've also expanded the sanitation module with sanitize_in_tensor (verify that input is torch.Tensor), sanitize_lshape (for larray.setter, see above), and I've modified constants.sanitize_infinity to use torch.finfo/iinfo and moved it tosanitation.sanitize_infinity.

Type of change

• New feature (non-breaking change which adds functionality)

Due Diligence

• All split configurations tested
• Multiple dtypes tested in relevant functions
• Documentation updated (if needed)
• Updated changelog.md under the title "Pending Additions"

Does this change modify the behaviour of other functions? If so, which?

no

[codecov]

Codecov Report

Merging #687 (8992946) into master (017ab4d) will increase coverage by 0.02%.
The diff coverage is 99.50%.

@@            Coverage Diff             @@
##           master     #687      +/-   ##
==========================================
+ Coverage   97.52%   97.54%   +0.02%     
==========================================
  Files          87       87              
  Lines       17746    17698      -48     
==========================================
- Hits        17306    17264      -42     
+ Misses        440      434       -6     

Impacted Files	Coverage Δ
heat/core/constants.py	100.00% <ø> (ø)
heat/core/linalg/qr.py	99.28% <ø> (ø)
heat/core/linalg/tests/test_basics.py	100.00% <ø> (ø)
heat/core/types.py	94.84% <ø> (ø)
heat/core/linalg/basics.py	94.25% <92.85%> (-0.22%)	⬇️
heat/core/_operations.py	96.73% <100.00%> (+3.12%)	⬆️
heat/core/dndarray.py	96.44% <100.00%> (+0.05%)	⬆️
heat/core/factories.py	99.11% <100.00%> (+0.06%)	⬆️
heat/core/indexing.py	100.00% <100.00%> (ø)
heat/core/io.py	90.39% <100.00%> (+0.06%)	⬆️
... and 14 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 017ab4d...8992946. Read the comment docs.

[Markus-Goetz]

Why do we introduce this property?

is_balanced should not have side-effects. It should be a pure informative call.

[ClaudiaComito]

Why do we introduce this property?

is_balanced should not have side-effects. It should be a pure informative call.

Hi Markus, I forgot to link the original problem, I've added it now (#668). Let me know if I should expand more.

[coquelin77]

looks good to me. i left a few questions in the code.

also, why did you change some of the test case to add an axis? where the other cases already covered? (i didnt look to deeply into this)

[Markus-Goetz]

@ClaudiaComito I understand the issue now. However, shouldn't setitem and getitem force consistency in this case? Moreover, I am still maintaining that a purely informative check called is_balanced should not cause side effects, i.e. occasionally do something behind the scenes.

[ClaudiaComito]

@ClaudiaComito I understand the issue now. However, shouldn't setitem and getitem force consistency in this case? Moreover, I am still maintaining that a purely informative check called is_balanced should not cause side effects, i.e. occasionally do something behind the scenes.

Oh, I see what you mean re: is_balanced. Yes, you're right. I'll remove the property setting then, but are you OK with the force_checksolution? (might be redundant if we get stricter about what larray.setter is allowed to do, see discussion above).

Regarding setitem and getitem. My idea was to have chunk check the balanced property and, if False, return the "measured" result instead of the theoretical one based on the balanced=True assumption. In that case, setitem and getitem would automatically get the correct result from chunk no matter what.