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.

Sign up for GitHub

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

Jump to bottom

Added Median of Medians method of finding median #33

Merged
merged 15 commits into from
Jun 26, 2018
Merged

Added Median of Medians method of finding median #33

merged 15 commits into from
Jun 26, 2018

Conversation

h-sinha
Copy link
Contributor

@h-sinha h-sinha commented Jun 23, 2018 •
edited
Loading

Fixes #11 Median Finding

sahilbansal17
sahilbansal17 reviewed Jun 23, 2018
View reviewed changes
Copy link
Owner

@sahilbansal17 sahilbansal17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code seems perfect.
Can you just provide a comment at the end of the program about the complexity of this algorithm?

@h-sinha
Copy link
Contributor Author

h-sinha commented Jun 23, 2018

Should I explain why the complexity is O(n) or just mentioning the complexity would suffice?

@sahilbansal17
Copy link
Owner

@h-sinha It would be nice if you could write a brief explanation.

ramitsawhney27
ramitsawhney27 requested changes Jun 23, 2018
View reviewed changes
Copy link
Collaborator

@ramitsawhney27 ramitsawhney27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add an explanation for choosing sublists of 5. Why didn't you use 4 or 6? Give an explanation for how you arrived at the time complexity, that's the main purpose of solving this problem.

@h-sinha
Copy link
Contributor Author

h-sinha commented Jun 23, 2018 •
edited
Loading

I have added the complexity and related explanation @sahilbansal17 @ramitsawhney27

sahilbansal17
sahilbansal17 requested changes Jun 24, 2018
View reviewed changes
Algorithms/Divide-and-Conquer/MedianFinding.cpp Outdated
else
{
//as size of median array is large find median of medians
pivot = findmedian(median, partition, partition/2);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@h-sinha This call would not work since you have renamed the function. Please run your program and make changes.

@h-sinha
Copy link
Contributor Author

h-sinha commented Jun 24, 2018

@sahilbansal17 updated

sahilbansal17
sahilbansal17 reviewed Jun 24, 2018
View reviewed changes
Copy link
Owner

@sahilbansal17 sahilbansal17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you just commit the a.out file? Remove it using git rm and commit.

@h-sinha
Copy link
Contributor Author

h-sinha commented Jun 24, 2018

It was removed in the last commit

arnav-mandal1234
arnav-mandal1234 requested changes Jun 24, 2018
View reviewed changes
Algorithms/Divide-and-Conquer/MedianFinding.cpp Outdated

#include<bits/stdc++.h>
using namespace std;
#define sz(a) int(a.size())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove these define statements.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arnav-mandal1234 updated

@sahilbansal17
Copy link
Owner

@ramitsawhney27 @arnav-mandal1234 I have approved this PR. Please review it once.

ramitsawhney27
ramitsawhney27 requested changes Jun 25, 2018
View reviewed changes
Copy link
Collaborator

@ramitsawhney27 ramitsawhney27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solve the recurrence, preferably not by substitution and show how you arrived at O(n).

@h-sinha
Copy link
Contributor Author

h-sinha commented Jun 25, 2018

@ramitsawhney27 Is it fine now?

@sahilbansal17
Copy link
Owner

@ramitsawhney27 Please check this PR.

@sahilbansal17 sahilbansal17 merged commit c03922c into sahilbansal17:master Jun 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arnav-mandal1234 arnav-mandal1234 arnav-mandal1234 requested changes

@sahilbansal17 sahilbansal17 sahilbansal17 approved these changes

@ramitsawhney27 ramitsawhney27 ramitsawhney27 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@h-sinha @sahilbansal17 @ramitsawhney27 @arnav-mandal1234