New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes: Negative indexing for Array is inconsistent #14678 #14679

Open
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@yeshsurya
Copy link

yeshsurya commented May 1, 2018

Fixes "Negative indexing for Array is inconsistent #14678"
When the indexes are negative initial value of real_index is not zero, it should increase. Scenario where index is positive will work as it was previous to fix.

@Abdullahjavednesar

This comment has been minimized.

Copy link
Contributor

Abdullahjavednesar commented May 2, 2018

@yeshsurya please add a test case.

@@ -79,6 +79,8 @@ def _parse_index(self, index):
for i in range(self._rank):
if index[i] >= self.shape[i]:
raise ValueError('Index ' + str(index) + ' out of border')
if(index[i]<0):
real_index+=1

This comment has been minimized.

@sidhantnagpal

sidhantnagpal May 3, 2018

Member

Spaces should be added before and after <, similarly for +=.
Also braces can be excluded for a single condition in if.

@yeshsurya

This comment has been minimized.

Copy link

yeshsurya commented May 12, 2018

@Abdullahjavednesar , Could you please suggest the where to add the test case. Is it like new test case file has be added or we need to add in any existing test case file. Also if a new test case file has to be created then is it in "tensor/tests" or "tensory/array/tests"

@sidhantnagpal

This comment has been minimized.

Copy link
Member

sidhantnagpal commented May 12, 2018

A new file might be required tensor/array/tests/test_ndim_array.py (as it currently does not exist).
See test file for the format.

@Abdullahjavednesar

This comment has been minimized.

Copy link
Contributor

Abdullahjavednesar commented May 12, 2018

May be @Upabjojr can help here.

@Upabjojr

This comment has been minimized.

Copy link
Contributor

Upabjojr commented May 15, 2018

There is no boundary check for the negative indices. Do we need one?

@yeshsurya

This comment has been minimized.

Copy link

yeshsurya commented May 21, 2018

@Upabjojr ,You are talking about whether to fix the issue or if test case is required ?

@Upabjojr

This comment has been minimized.

Copy link
Contributor

Upabjojr commented May 21, 2018

Test is required. Issue may be fixed here or in another PR. But better here.

@yeshsurya

This comment has been minimized.

Copy link

yeshsurya commented May 27, 2018

Added tests related to Issue #14679

@sidhantnagpal

This comment has been minimized.

Copy link
Member

sidhantnagpal commented May 27, 2018

Fixes "Negative indexing for Array is inconsistent #14678"

To use the autoclose format for the issue use keywords immediately before the issue number: "Fixes #14678" or "Closes #14678".
See here for more information.

Yeshwanth N and others added some commits May 27, 2018

Yeshwanth N
Yeshwanth N


def test_array_negative_indices():
test_array = Array([[1,2,3,4,5],[6,7,8,9,10]])

This comment has been minimized.

@Upabjojr

Upabjojr Jun 10, 2018

Contributor

We use the standardized 4 spaces indentation in SymPy.

By the way, maybe this should be put into the other test files? I don't see the point of creating a new file.

This comment has been minimized.

@yeshsurya

yeshsurya Jun 10, 2018

@sidhantnagpal felt tests were to be in new file.So, new file was added. @Upabjojr , could you please help me in the build failure, for there is failure in first assertion. test_array[:,-1] is [10,5]. However the assertion for the first one is failing, not able to figure out the reason.

This comment has been minimized.

@Upabjojr

Upabjojr Jun 11, 2018

Contributor

did you debug?

This comment has been minimized.

@yeshsurya

yeshsurya Jul 14, 2018

I tested in my local machine, The assertion is "True" for the line to which build is marking as failure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment