Why is it needed to change prior tests? Were they incorrect?

@moorepants Yes, they were incorrect. Since earlier, even if the index went out of bound (like in the above case, -4, 4, and 5 were out of bounds) or was a non integer, it didn't result in an error (which it should've). But that's been corrected by this PR.

I think the existing tests are correct, as the indices are being pivoted to the appropriate range. You would want integer casts for pos using as_int for an exception be raised. (I think that is what Aaron was pointing out, that current approach won't affect float inputs.)

@sidhantnagpal I have corrected the floats part (which @asmeurer had asked) but I still don't understand why is the range (-4, 5) valid? I think the entire purpose of using this statement :

        if pos < 0:
            pos = self.rows + pos

was

1. to make the negative indices >=0
   and
2. pos when exceeds self.rows, doesn't make sense to insert another row in place of a row which doesn't exist. This is why I think -4, 4, and 5 are out of bounds.
   Am I wrong here?
   What do you think?

@sidhantnagpal ping

I think these lines should be left as-is (and exception should not be raised).
This behaviour is consistent with the way indexing works in Python.
The only change could be i = as_int(pos) (with the necessary import).

@sidhantnagpal

The only change could be i = as_int(pos) (with the necessary import).

If I am not raising an error (IndexError), what good will that do?

I think these lines should be left as-is (and exception should not be raised).

Why do you think a float input should not result in an error?

Those lines don't handle float inputs specially.
The conditional checks could use i instead of pos.
The exception will then be raised in as_int (and not anywhere else).

IndexError is for out-of-bound errors, TypeError should be used here (that too if an exception has to be raised explicitly).

@sidhantnagpal Okay, will do.

This else seems redundant and also causes an extra indentation level.

@sidhantnagpal Okay, will do.

This change is not required.

@sidhantnagpal Okay, will do.

I think the code is well-tested viz. test_row_inserts() and test_col_inserts(). The error tests could probably go in test_errors().

@sidhantnagpal Okay, will do.

Is there a reason to prefer that TypeError instead of ValueError be raised? If not, this could just be pos = as_int(pos) and the standard ValueError would be raised...and it should give enough of a clue as to what is wrong.

@smichr the purpose of using the TypeError was to clarify to the user what exactly has he done wrong and what is the correct format. ValueError only shows pos is not an integer while TypeError also shows what is the correct format.

@smichr What do you think? If you insist, I'l remove the try: statement and just keep it as pos = as_int(pos) in the beginning. What do you say?

I think we should not create custom error messages if a standard response is sufficient. e.g.

>>> factorint(3.5)
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/base/data/home/apps/s~sympy-live-hrd/55.412087474737415211/sympy/sympy/ntheory/factor_.py", line 1029, in factorint
    n = as_int(n)
  File "/base/data/home/apps/s~sympy-live-hrd/55.412087474737415211/sympy/sympy/core/compatibility.py", line 377, in as_int
    raise ValueError('%s is not an integer' % (n,))
ValueError: 3.5 is not an integer

The user learns from this that 3.5 is not an integer. They also know that this is happening in factorint which they just called. So saying that "factorint expects an integer" is redundant. My feeling is that the same applies here. Any routine that expects to process integers and needs to raise an error for non-integer intput can safely put n = as_int(n) at the top of their routine and know that no non-int will pass and the user will know of the issue.