Skip to content
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

Using DynamicOneDimensionalArray in Arraystack #69

Merged
merged 9 commits into from
Dec 30, 2019

Conversation

Saptashrungi
Copy link
Contributor

@Saptashrungi Saptashrungi commented Dec 28, 2019

Fixes #67.

@czgdp1807
Copy link
Member

The API is breaking by your changes, that's why the tests are failing. We need to find out a way to keep the API intact while using DynamicOneDimensionalArray.

@czgdp1807
Copy link
Member

czgdp1807 commented Dec 28, 2019

I think we need only two data members, items and dtype.
ArrayStack.__new__ will change to,

class ArrayStack(...):
    def __new__(cls, items=None, dtype=int):
        ...

Or in fact,

class ArrayStack(...):
    def __new__(cls, dtype=int):
        ...

items are not needed in parameters.

@Saptashrungi
Copy link
Contributor Author

Saptashrungi commented Dec 28, 2019 via email

@czgdp1807
Copy link
Member

items = DynamicOneDimensionalArray(dtype, size)

Agreed. In fact, it should be DyamicOneDimensionalArray(dtype, 0).

@Saptashrungi
Copy link
Contributor Author

Saptashrungi commented Dec 28, 2019 via email

@czgdp1807
Copy link
Member

Currently the size doesn't defaults to 0, so you have to give 0 as the second argument.

>>> from pydatastructs import DynamicOneDimensionalArray as DODA
>>> arr = DODA(int)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/czgdp1807/codezonediitj/pydatastructs/pydatastructs/linear_data_structures/arrays.py", line 207, in __new__
    obj = super().__new__(cls, dtype, *args, **kwargs)
  File "/home/czgdp1807/codezonediitj/pydatastructs/pydatastructs/linear_data_structures/arrays.py", line 70, in __new__
    raise ValueError("1D array cannot be created due to incorrect"
ValueError: 1D array cannot be created due to incorrect information.

@czgdp1807
Copy link
Member

We can change the behaviour after some discussion but in a different PR.

@Saptashrungi
Copy link
Contributor Author

Saptashrungi commented Dec 28, 2019 via email

@czgdp1807
Copy link
Member

We can change the behaviour after some discussion but in a different PR.

As said above you can do that, but in a different PR(from a different branch).

@Saptashrungi
Copy link
Contributor Author

Saptashrungi commented Dec 28, 2019 via email

@czgdp1807
Copy link
Member

Thanks for the contributions first of all. It's a great experience to work with you. Hoping that you will keep communicating and contributing in this way in future too. :-)

@Saptashrungi
Copy link
Contributor Author

Saptashrungi commented Dec 28, 2019 via email

@czgdp1807
Copy link
Member

czgdp1807 commented Dec 29, 2019

Please update the documentation as well for the new API. The tests failing due to that.
Please see if Stack is used in other modules, update the usage there as well. As far as I remember, BinaryTreeTraversal uses Stack so update is needed there too.

@Saptashrungi
Copy link
Contributor Author

Saptashrungi commented Dec 29, 2019 via email

@czgdp1807
Copy link
Member

Yes, the comments in the code. I have made some edits in my previous comments which should be visible if you open github.

@Saptashrungi
Copy link
Contributor Author

Saptashrungi commented Dec 29, 2019 via email

@czgdp1807
Copy link
Member

Please make sure that your code passes the recently added code quality tests. See the logs here. There are few trailing white spaces in your code.

@czgdp1807
Copy link
Member

To test your code quality locally, update the master branch by pulling the latest changes and merge into doda branch of this PR.

@codecov
Copy link

codecov bot commented Dec 29, 2019

Codecov Report

Merging #69 into master will decrease coverage by 0.498%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##           master       #69       +/-   ##
============================================
- Coverage   98.17%   97.671%   -0.499%     
============================================
  Files          19        23        +4     
  Lines        1257      1503      +246     
============================================
+ Hits         1234      1468      +234     
- Misses         23        35       +12
Impacted Files Coverage Δ
.../miscellaneous_data_structures/tests/test_stack.py 100% <100%> (ø) ⬆️
...datastructs/miscellaneous_data_structures/stack.py 90.243% <100%> (-2.064%) ⬇️
pydatastructs/trees/binary_trees.py 96.437% <100%> (ø) ⬆️
pydatastructs/trees/heaps.py 97.575% <0%> (-1.016%) ⬇️
pydatastructs/utils/misc_util.py 94.871% <0%> (-0.781%) ⬇️
pydatastructs/utils/__init__.py 100% <0%> (ø) ⬆️
...astructs/miscellaneous_data_structures/__init__.py 100% <0%> (ø) ⬆️
pydatastructs/trees/__init__.py 100% <0%> (ø) ⬆️
pydatastructs/trees/tests/test_heaps.py 100% <0%> (ø) ⬆️
...ts/miscellaneous_data_structures/binomial_trees.py 100% <0%> (ø)
... and 3 more

Impacted file tree graph

@czgdp1807
Copy link
Member

Thanks. Merging.

@czgdp1807 czgdp1807 merged commit 23c9e91 into codezonediitj:master Dec 30, 2019
@czgdp1807
Copy link
Member

If you want you can add your name to https://github.com/codezonediitj/pydatastructs/blob/master/AUTHORS by making a separate PR.

@czgdp1807 czgdp1807 mentioned this pull request Dec 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use DynamicOneDimensionalArray in ArrayStack
2 participants