Validates number of dimensions in input to ht.sparse.sparse_csr_matrix

[Ishaan-Chandak]

Description

ht.sparse.sparse_csr_matrix should accept only 2-D input. But it accepts (and crashes) when the input is not 2 dimensional.

The input must be properly validated to ensure only 2-D input is accepted. Or else an error with the message "The number of dimensions must be 2, found ndim" must be raised.

Changes proposed

Using tensor.ndim to find the dimension of the tensor and then using if-else statements to check the dimension of the input and raise the exception.

Fixes: #1091

[Mystic-Slice]

@Ishaan-Chandak Good work so far!
But there is a small problem with this fix. This will allow tensors with layout torch.sparse_csr that are not 2-dimensional. We have to prevent that too.

For example, try running this code,

import torch
import heat as ht

a = torch.tensor([[[0, 1]]], dtype=torch.float64).to_sparse_csr()
t = ht.sparse.sparse_csr_matrix(a)
print(t + t)

The number of dimensions has to be checked irrespective of the layout of the input object.
So, check the number of dimensions before checking the layout attribute.

[Mystic-Slice]

Try to provide some supporting unit tests for this change once you fix this.
And of course, don't hesitate to ask here if you have any doubts.

[Ishaan-Chandak]

Hi, @Mystic-Slice I made the necessary changes please review the PR, I also ran all the unittests as mentioned in the documentation provided.
Thank you

[Ishaan-Chandak]

I am extremely sorry for this commit I messed up the code at a point I ll make the changes as soon as possible
Sorry @Mystic-Slice

[Mystic-Slice]

@Ishaan-Chandak New tests need to be added to the existing unit-tests to ensure the check for dimensionality works properly.
You can create the new tests in heat/core/tests/test_factories.py

[Ishaan-Chandak]

I ll be adding the tests soon @Mystic-Slice

[Ishaan-Chandak]

Hi @Mystic-Slice
`
def test_dimensions(self):
# for a two dimension tensor no error is raised

    # more than two dimension tensors with sparse_csr layout 
    a = torch.tensor([[[0, 1]]], dtype=torch.float64).to_sparse_csr()
    self.assertRaises(ValueError, ht.sparse.sparse_csr_matrix, a)

    # for one dimension 
    self.assertRaises(ValueError, ht.sparse.sparse_csr_matrix, torch.tensor([0, 1]))
    self.assertRaises(ValueError, ht.sparse.sparse_csr_matrix, torch.tensor([1, 0, 3]))
    self.assertRaises(ValueError, ht.sparse.sparse_csr_matrix, torch.tensor([ ]))
    self.assertRaises(ValueError, ht.sparse.sparse_csr_matrix, torch.tensor([1, 0, 3, 4]))

    # for more than one dimension
    self.assertRaises(ValueError, ht.sparse.sparse_csr_matrix, torch.tensor([[[1, 0, 3]]]))

`
Are these many unit-tests enough ?

[Mystic-Slice]

@Ishaan-Chandak One test for 1-D input and one test for > 2-D input is enough. Add these tests at the end of test_sparse_csr_matrix method. No need to create a new function.
Also, I linked the wrong file in my previous comment. Sorry about that. These tests are to be added to heat/sparse/tests/test_factories.py.

[Ishaan-Chandak]

@Mystic-Slice Is test for 2-D input necessary because when the input is 2-D the programs keeps on running without any interruptions.
So according to me it will be better to have two inputs one for greater than 2 and one for less than 2

[Mystic-Slice]

Yes, I meant exactly that by > 2-D (greater than 2-D). :)

[Ishaan-Chandak]

Hi @Mystic-Slice I have added the tests and made the changes as you told me to do please review the commit.

[Ishaan-Chandak]

Hi @Mystic-Slice Can you please review this and tell me if any changes are needed.
Thank you

[Mystic-Slice]

Looks good to me

[Mystic-Slice]

@ClaudiaComito This PR is good to be merged. The pre-commit ci failure is not related to this PR.

And @Ishaan-Chandak Nice work!

[ClaudiaComito]

Wonderful, thanks a lot @Ishaan-Chandak and @Mystic-Slice !

[ClaudiaComito]

This PR needs CI. Waiting on developments in #1114

[Ishaan-Chandak]

Hi @ClaudiaComito I am unable to understand what should I do now ? And can you please explain what exactly is CI

[Mystic-Slice]

@Ishaan-Chandak CI is Continuous Integration. It is the tests that run before the PR is allowed to be merged. You might have noticed that some tests do not run in your PR. That is because CI, as of now, does not work on external contributor forks. And the work to enable CI for external forks is going on in #1114. Your work here is complete. We will merge the PR once it is ready. Thanks for the fix. We will take care of the rest.

tldr, You do not need to do anything.

[ClaudiaComito]

Hi @ClaudiaComito I am unable to understand what should I do now ? And can you please explain what exactly is CI

@Ishaan-Chandak oh I'm so sorry, that was more of an internal communication 😬 apologies. @Mystic-Slice thanks for explaining. Once we can run the tests, we'll know if more needs to be done. Nothing you can do about this @Ishaan-Chandak , thanks again for your time!

[Ishaan-Chandak]

Hi @Mystic-Slice @ClaudiaComito I received a mail regarding some tests failing due to older version of heat so should i do something to rectify them ?

[github-actions]

Thank you for the PR!

[codecov]

Codecov Report

Merging #1098 (c12297e) into main (d744909) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1098   +/-   ##
=======================================
  Coverage   91.80%   91.80%           
=======================================
  Files          72       72           
  Lines       10483    10485    +2     
=======================================
+ Hits         9624     9626    +2     
  Misses        859      859           

Flag	Coverage Δ
unit	91.80% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
heat/sparse/factories.py	95.74% <100.00%> (+0.09%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[ClaudiaComito]

Hi @Mystic-Slice @ClaudiaComito I received a mail regarding some tests failing due to older version of heat so should i do something to rectify them ?

@Ishaan-Chandak no that was still our old tests. We have merged the new ones now, I'm about to merge this PR. 👏🏼 Thanks @Ishaan-Chandak @mtar and @Mystic-Slice for all the work!