-
Notifications
You must be signed in to change notification settings - Fork 269
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
Multi-dimensional array added #256
Conversation
Added .idea files
On multi-dimensionals arrays removed unused code and finished __getitem__
Fixed issues on setitem
Fixed MDA when initializing with 1 dimension Fixed MDA Fill function Changed ODA according to PEP 8 (mostly identation)
Added MDA test
Changed __new__ in MDA to work with recursion Removed testing code
Changed MDA test
Changed docstring of MDA
Codecov Report
@@ Coverage Diff @@
## master #256 +/- ##
=========================================
Coverage 98.842% 98.842%
=========================================
Files 23 23
Lines 2420 2420
=========================================
Hits 2392 2392
Misses 28 28 |
if dtype is NoneType or len(args) == (0): | ||
raise ValueError("array cannot be created due to incorrect" | ||
" information.") | ||
dimensions = list(args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this variable for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable is to store the sizes of the different dimensions, and used to set the size of the dimension
lets say you want to create an array of three dimensions, each dimension with its own size like 4 3 2
So,
MultiDimensionalArray(int, 4, 3, 5)
then
dimensions == [4, 3, 5]
I used it mostly for convenience since i use pop and I wanted to have an easier name, but if you want me to only use args, i can change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it appears to me that this uses recursive solution i.e., array of arrays of arrays for three dimensions. What we want is, an internal 1D layout with size as product of dimensions with RMO being used to convert queries like, M[i, j, k] to M[l], where l = RMO(i, j, k).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, i did it again, this time it only uses a one dimensional array, calculating the position with the size of each dimension
Are you participating through GSSoC, 2020? |
No, is there something important i should know? |
No, but we ask because GSSoC have their auto-grader for PRs made by it's participants. Labels have to be applied for GSSoC PRs. That's all. You can keep working irrespective of your participation in any open source programs. |
it now uses a single one dimensional array
fixed sizes calculation and expanded test
Updated docstring of MDA
Fixed size calculation of MDA added 1 step to the test
if dtype is NoneType or len(args) == (0): | ||
raise ValueError("array cannot be created due to incorrect" | ||
" information.") | ||
dimensions = list(args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dimensions = list(args) | |
dimensions = args |
raise ValueError("array cannot be created due to incorrect" | ||
" information.") | ||
dimensions = list(args) | ||
if dimensions[0] <= 1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if dimensions[0] <= 1: | |
if dimensions[0] < 1: |
1
should be fine. Why only checked for dimensions[0]
? Is, dimensions[2] = 0
allowed?
Co-authored-by: Gagandeep Singh <[email protected]>
adding __str__ in MDA
|
||
def test_MultiDimensionalArray(): | ||
array = MultiDimensionalArray(int, 5, 9, 3, 8) | ||
assert array.shape == (5, 9, 3, 8) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't working as expected. array.shape
is giving, (2560, 576, 24, 8, 1)
. See, https://travis-ci.com/github/codezonediitj/pydatastructs/builds/164219241 for details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JeanPierreMR Can you please look into the issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sure
the idea of shape is that it saves the size of each slice of array.
In this case the 2560 is the full size of the array, 576 its the size of the slices of the first dimension.... 8 is the size of the second to last dimension, and 1 is the last one
the point of having it like this is to avoid calculating it every time you want to access something.
I could:
- make 2 variables to save both, one called shape and one sizes (I think this would be the best)
- calculate the (5, 9, 3, 8), from the (2560, 576, 24, 8, 1) to access it
- calculate the size (2560, 576, 24, 8, 1) every time you want to access something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Earlier we weren't saving the shape
of the array and only sizes
were being used? So, may be shape is just needed when .shape
is called. We can go with, the second approach.
In addition, you can rename, _shape
to _sizes
as I got your point of using it.
Use typing module in your patch to avoid type checkings. See, #276 |
Fixed shape on MDA
updated test MDA
Looks good. I will push some changes in a while for type-annotations, just enable, |
sure, it is already checked :) |
Thanks. |
Fixes #104
Multi-dimensional array added
Some changes to one dimensional array in order to follow PEP 8, nothing more than spacing