Fixed initialization of DNDarrays communicator in some routines

[AsRaNi1]

Description

Previously some HeAT routines, e.g. matmul, the initialization of new DNDarrays (usually the output of the routine) was done without specifying a communicator for the new DNDarray properly.
In matmul the output array c = a @ b is initialized as

c = factories.zeros(c_shape, split=a.split, dtype=c_type, device=a.device)

I have added comm=a.comm here and at other comparable places
Issue/s resolved: #1074

Changes proposed:

Previously gave error for the code snippet in #1074

Type of change

Bug fix

Due Diligence

• All split configurations tested
• Multiple dtypes tested in relevant functions
• Documentation updated (if needed)
• Title of PR is suitable for corresponding CHANGELOG entry

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

no

[AsRaNi1]

@ClaudiaComito I am not understanding how to initialise codecov

[Markus-Goetz]

@AsRaNi1 thank you for the addition to the code base. Do you mind fixing the merge issues?

[ClaudiaComito]

@AsRaNi1 indeed, thank you so much.

We have been discussing the issue of activating codecov for external/first-time contributors, @mtar is looking into it.

In the meantime:

• There are many more instances in the codebase where factories.array() is initiated without specifying the communicator. I know the Issue didn't mention those, but would you consider fixing those as well? It would be awesome.

• how do we test this fix? @mrfh92 @Markus-Goetz

Thanks all!

[mrfh92]

• As a further examples: I think at percentile and resplit there is the same problem
• I think testing is not difficult (from an conceptual point of view): in principle, it suffices to test any routine that has been tested on the default communicator MPI.COMM_WORLD on a subcommunicator as well. Unfortunately, this will roughly double the time the CI needs if one just adds a second test on roughly "half of MPI.COMM_WORLD" (or sth similar) ... otherwise one could just replace the tests on MPI.COMM_WORLD by tests on a subcommunicator only. The latter should suffice to catch all potential errors but I dont like the idea of not testing the most important case explicitely...

[ClaudiaComito]

• As a further examples: I think at percentile and resplit there is the same problem

  • I think testing is not difficult (from an conceptual point of view): in principle, it suffices to test any routine that has been tested on the default communicator MPI.COMM_WORLD on a subcommunicator as well. Unfortunately, this will roughly double the time the CI needs if one just adds a second test on roughly "half of MPI.COMM_WORLD" (or sth similar) ... otherwise one could just replace the tests on MPI.COMM_WORLD by tests on a subcommunicator only. The latter should suffice to catch all potential errors but I dont like the idea of not testing the most important case explicitely...

I was more thinking of a test in which some operation is performed on a subset of the available processes.

[mrfh92]

This is exactly what I meant, but maybe I expressed it a bit lengthy and unclear

[ClaudiaComito]

This is exactly what I meant, but maybe I expressed it a bit lengthy and unclear

No problem, my main point is we don't need to test this on every operation. For example, in the test_communication module we could have a test_subcomm function that runs a matrix multiplication on 2 processes only.

[AsRaNi1]

@AsRaNi1 indeed, thank you so much.

We have been discussing the issue of activating codecov for external/first-time contributors, @mtar is looking into it.

In the meantime:

• There are many more instances in the codebase where factories.array() is initiated without specifying the communicator. I know the Issue didn't mention those, but would you consider fixing those as well? It would be awesome.
• how do we test this fix? @mrfh92 @Markus-Goetz

Thanks all!

Yes sure!
I'll look into it
Thank you!

[AsRaNi1]

@Markus-Goetz please review the following PR

[AsRaNi1]

heat/nn/data_parallel.py:244 in private method `_nonblocking_hook`:
        D202: No blank lines allowed after function docstring (found 1)
heat/nn/data_parallel.py:282 in private method `_forward_hook`:
        D202: No blank lines allowed after function docstring (found 1)

The following error is coming in pre-commit.ci while testing
I tried to make the changes in data_parallel.py but black reformats the file then pydocstyle throws the error

[ClaudiaComito]

heat/nn/data_parallel.py:244 in private method `_nonblocking_hook`:
        D202: No blank lines allowed after function docstring (found 1)
heat/nn/data_parallel.py:282 in private method `_forward_hook`:
        D202: No blank lines allowed after function docstring (found 1)

The following error is coming in pre-commit.ci while testing I tried to make the changes in data_parallel.py but black reformats the file then pydocstyle throws the error

@AsRaNi1 #1103 is in the queue and will be merged into main when ready. Please update your branch afterwards and at least the pre-commit failure should be fixed. Thanks again @Mystic-Slice !

[AsRaNi1]

heat/nn/data_parallel.py:244 in private method `_nonblocking_hook`:
        D202: No blank lines allowed after function docstring (found 1)
heat/nn/data_parallel.py:282 in private method `_forward_hook`:
        D202: No blank lines allowed after function docstring (found 1)

The following error is coming in pre-commit.ci while testing I tried to make the changes in data_parallel.py but black reformats the file then pydocstyle throws the error

@AsRaNi1 #1103 is in the queue and will be merged into main when ready. Please update your branch afterwards and at least the pre-commit failure should be fixed. Thanks again @Mystic-Slice !

I have updated the branch

[github-actions]

Thank you for the PR!

[mrfh92]

Unfortunately, the detailed results of the CI are not available to externals, so I post the result here:
The CI found an error, most likely in resplit (?):

>           new_arr = factories.array(
                gathered, is_split=axis, device=arr.device, arr=arr.comm, dtype=arr.dtype
            )
E           TypeError: array() got an unexpected keyword argument 'arr'
heat/core/manipulations.py:3387: TypeError

[github-actions]

Thank you for the PR!

[AsRaNi1]

Unfortunately, the detailed results of the CI are not available to externals, so I post the result here: The CI found an error, most likely in resplit (?):

>           new_arr = factories.array(
                gathered, is_split=axis, device=arr.device, arr=arr.comm, dtype=arr.dtype
            )
E           TypeError: array() got an unexpected keyword argument 'arr'
heat/core/manipulations.py:3387: TypeError

I'll make the changes!

[github-actions]

Thank you for the PR!

[github-actions]

Thank you for the PR!

[github-actions]

Thank you for the PR!

[github-actions]

Thank you for the PR!

[github-actions]

Thank you for the PR!

[mrfh92]

@mtar The CI failed for AMD and CUDA and actually I dont know whats the problem because no heat-related errors are reported... is it something regarding the CI?

[github-actions]

Thank you for the PR!

[github-actions]

Thank you for the PR!

[mtar]

@mtar The CI failed for AMD and CUDA and actually I dont know whats the problem because no heat-related errors are reported... is it something regarding the CI?

fixed

[mrfh92]

From my point of view, the changes are fine. Therefore I recommend merging, since the CI runs through as well.

[mrfh92]

@ClaudiaComito From my point of view, this PR is ready to merge. Do you agree?

[ClaudiaComito]

@ClaudiaComito From my point of view, this PR is ready to merge. Do you agree?

@mrfh92 Yes, please go ahead as soon as the CI is through. Congrats @AsRaNi1 and thanks for your contribution!

[mrfh92]

@AsRaNi1 Thanks for your work! :)