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

feat : add fenwick tree 2D data structure, struct fenwick2D #1230

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

zyrch
Copy link

@zyrch zyrch commented Oct 8, 2020

Description of Change

Checklist

  • Added description of change
  • Added file name matches File name guidelines
  • Added tests and example, test must pass
  • Added documentation so that the program is self-explanatory and educational - Doxygen guidelines
  • Relevant documentation/comments is changed or added
  • PR title follows semantic commit guidelines
  • Search previous suggestions before making a new one, as yours may be a duplicate.
  • I acknowledge that all my contributions will be made under the project's license.

Notes:

@kvedala kvedala added automated tests are failing Do not merge until tests pass Proper Documentation Required requested to write the documentation properly labels Oct 8, 2020
@kvedala
Copy link
Collaborator

kvedala commented Oct 8, 2020

@zyrch zyrch changed the title Create fenwick_tree_2D.cpp fest : add fenwick tree 2D data structure, struct fenwick2D Oct 9, 2020
@zyrch
Copy link
Author

zyrch commented Oct 9, 2020

Changes have been made.

@zyrch zyrch changed the title fest : add fenwick tree 2D data structure, struct fenwick2D feat : add fenwick tree 2D data structure, struct fenwick2D Oct 10, 2020
@zyrch zyrch closed this Oct 10, 2020
@zyrch zyrch reopened this Oct 10, 2020
ayaankhan98
ayaankhan98 previously approved these changes Oct 12, 2020
@ayaankhan98
Copy link
Member

code looks fine but documentation is not up to standard

@ayaankhan98 ayaankhan98 reopened this Oct 12, 2020
@Panquesito7 Panquesito7 added approved Approved; waiting for merge enhancement New feature or request labels Oct 12, 2020
@zyrch
Copy link
Author

zyrch commented Oct 13, 2020

changes have been made in the documentation

@Panquesito7 Panquesito7 removed the automated tests are failing Do not merge until tests pass label Oct 13, 2020
Copy link
Member

@Panquesito7 Panquesito7 left a comment

Choose a reason for hiding this comment

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

Documentation is still not up to the repository standards.
Please see the typical structure of a program, that might help. 🙂

@realstealthninja
Copy link
Collaborator

this seems to be stale

Comment on lines +1 to +3
#include <iostream>
#include <cassert>
#include <vector>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#include <iostream>
#include <cassert>
#include <vector>
#include <iostream> /// for I/O operations
#include <cassert> /// for assert
#include <vector> /// for std::vector

needs documentation

@@ -0,0 +1,129 @@
#include <iostream>
Copy link
Collaborator

Choose a reason for hiding this comment

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

document the header

Comment on lines +105 to +126
int n = 5;

fenwick2D<int> fenwick_tree_2D(n);

fenwick_tree_2D.update(1, 1, 2);
fenwick_tree_2D.update(1, 2, 3);
fenwick_tree_2D.update(2, 1, 3);
fenwick_tree_2D.update(1, 3, 4);
fenwick_tree_2D.update(2, 2, 4);
fenwick_tree_2D.update(3, 1, 4);
fenwick_tree_2D.update(1, 4, 5);
fenwick_tree_2D.update(2, 3, 5);
fenwick_tree_2D.update(3, 2, 5);
fenwick_tree_2D.update(3, 3, 6);

assert(fenwick_tree_2D.query(1, 1, 2, 2) == 12);
assert(fenwick_tree_2D.query(1, 2, 3, 3) == 27);
fenwick_tree_2D.update(1, 1, -3);
assert(fenwick_tree_2D.query(1, 1, 2, 2) == 9);
assert(fenwick_tree_2D.query(1, 2, 3, 3) == 27);

std::cout << "All Test Passed\n";
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be moved to a function

void test() {};

}

};

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/**
* @brief Main function
* @returns 0 on exit
*/

}

// Calculates a prefix sum till index idx
T sum(int idx) {
Copy link
Contributor

Choose a reason for hiding this comment

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

might want to check if idx <= siz


// update the values at idx by delta
// update all the indices which include idx in their sum
void update(int idx, T delta) {
Copy link
Contributor

Choose a reason for hiding this comment

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

similarly, could check the bounds of idx

}

// find sum in range l to r
T query(int l, int r) {
Copy link
Contributor

Choose a reason for hiding this comment

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

finally, here are even more implicit assumptions on the input, 0 < l <= r <= siz (I believe)

return an;
}

// sum of region {x1 to x} and {y1 to y}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: For clarity, I think it would be better to call them x1, x2 and y1, y2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved; waiting for merge enhancement New feature or request Proper Documentation Required requested to write the documentation properly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants