Skip to content

brick_sort implemented #207

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

Merged
merged 9 commits into from
Mar 25, 2020
Merged

Conversation

HarsheetKakar
Copy link
Contributor

@HarsheetKakar HarsheetKakar commented Mar 24, 2020

References to other Issues or PRs or Relevant literature

#129

Brief description of what is fixed or changed

Other comments

@codecov
Copy link

codecov bot commented Mar 24, 2020

Codecov Report

Merging #207 into master will increase coverage by 0.027%.
The diff coverage is 100.000%.

@@              Coverage Diff              @@
##            master      #207       +/-   ##
=============================================
+ Coverage   96.897%   96.924%   +0.027%     
=============================================
  Files           24        24               
  Lines         1934      1951       +17     
=============================================
+ Hits          1874      1891       +17     
  Misses          60        60               
Impacted Files Coverage Δ
pydatastructs/linear_data_structures/__init__.py 100.000% <ø> (ø)
pydatastructs/linear_data_structures/algorithms.py 100.000% <100.000%> (ø)

Impacted file tree graph

@HarsheetKakar HarsheetKakar requested a review from czgdp1807 March 24, 2020 19:56
@HarsheetKakar
Copy link
Contributor Author

so is it ready to merge now? @czgdp1807

@czgdp1807
Copy link
Member

so is it ready to merge now? @czgdp1807

Tests should be updated. Common tests should be used for all sorting algorithms. The function, test_merge_sort_parallel can be renamed to _test_common_sort(sort_func) and test_merge_sort_parallel and test_brick_sort should call them from inside, like,

def test_merge_sort_parallel():
    _test_common_sort(merge_sort_parallel)

@HarsheetKakar
Copy link
Contributor Author

so is it ready to merge now? @czgdp1807

Tests should be updated. Common tests should be used for all sorting algorithms. The function, test_merge_sort_parallel can be renamed to _test_common_sort(sort_func) and test_merge_sort_parallel and test_brick_sort should call them from inside, like,

def test_merge_sort_parallel():
    _test_common_sort(merge_sort_parallel)

sure sounds good, and while we are at it I also feel that we should put the algorithms under their own module.

@czgdp1807
Copy link
Member

Or testing function for parallel and non parallel algorithms can be separated(array(s) used in testing should be same, only num_threads needs attention). I hope you are getting my point.

@czgdp1807
Copy link
Member

while we are at it I also feel that we should put the algorithms under their own module.

Well, every algorithm.py file under different modules contains algorithms for the data structure represented by that module. This helps in keeping things specific for each module. Every algorithm is under global namespace so not a big issue.

@HarsheetKakar
Copy link
Contributor Author

Or testing function for parallel and non parallel algorithms can be separated(array(s) used in testing should be same, only num_threads needs attention). I hope you are getting my point.

I get it, and I have felt the same while I was working on it... so this is the proposal:

pydatastructs

algorithms

sequential

tests

concurrent

tests

@czgdp1807
Copy link
Member

czgdp1807 commented Mar 25, 2020

I get it, and I have felt the same while I was working on it... so this is the proposal:

Every parallel algorithm has _parallel appended to it, so putting them under different module isn't really useful . Though our current classes may not work with parallel algorithms through processes because sharing user defined objects among processes is not possible with python. A _to_python_object method can be introduced in every class to support that. Opening a separate issue for this.

@HarsheetKakar
Copy link
Contributor Author

Or testing function for parallel and non parallel algorithms can be separated(array(s) used in testing should be same, only num_threads needs attention). I hope you are getting my point.

take a look at it please.

@HarsheetKakar HarsheetKakar requested a review from czgdp1807 March 25, 2020 17:28
@czgdp1807 czgdp1807 merged commit 37a9483 into codezonediitj:master Mar 25, 2020
@HarsheetKakar HarsheetKakar deleted the bricksort branch March 25, 2020 19:24
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.

3 participants