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

Added sqrt decomposition #613

Merged
merged 20 commits into from
Jul 21, 2023
Merged

Conversation

lorenzotinfena
Copy link
Contributor

@lorenzotinfena lorenzotinfena commented Dec 2, 2022

Added the data structure like the one described here (not including mo's algorithm) https://cp-algorithms.com/data_structures/sqrt_decomposition.html.
But I made it more general as possible, using generic functions to query, merge 2 queries, and change the value of an element

@lorenzotinfena lorenzotinfena changed the title Added binary heap (heap) Added binary heap (heap) and sqrt decomposition simple Dec 2, 2022
Copy link
Contributor Author

@lorenzotinfena lorenzotinfena left a comment

Choose a reason for hiding this comment

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

this comment is a test

Copy link
Member

@tjgurwara99 tjgurwara99 left a comment

Choose a reason for hiding this comment

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

Please read our contribution guidelines. I have reviewed this but I was going to avoid it reviewing this PR because there is no context provided on what you're trying to do and achieve in this PR. You are expecting the maintainers to spend time figuring out most of your code when you haven't even provided a decent description (no description in fact). Just opening a PR without description is not a very good way of contributing to projects.

sqrt_decomposition/sqrt_decomposition_simple.go Outdated Show resolved Hide resolved
sqrt_decomposition/sqrt_decomposition_simple.go Outdated Show resolved Hide resolved
structure/heap/binary_heap.go Outdated Show resolved Hide resolved
structure/heap/binary_heap.go Outdated Show resolved Hide resolved
@lorenzotinfena
Copy link
Contributor Author

lorenzotinfena commented Dec 11, 2022

I noticed that in another pr somedid did heap, so I removed it. I also added a description at this pr

@lorenzotinfena lorenzotinfena changed the title Added binary heap (heap) and sqrt decomposition simple Added sqrt decomposition simple Jan 3, 2023
@lorenzotinfena
Copy link
Contributor Author

I fixed the style

@github-actions
Copy link

github-actions bot commented Feb 4, 2023

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the stale label Feb 4, 2023
@lorenzotinfena
Copy link
Contributor Author

(comment to not close this) I wish I can work on this when I have time

@tjgurwara99 tjgurwara99 removed the stale label Feb 7, 2023
@github-actions
Copy link

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the stale label Mar 10, 2023
@lorenzotinfena
Copy link
Contributor Author

(comment to not close this)

Copy link

@ankit-gautam23 ankit-gautam23 left a comment

Choose a reason for hiding this comment

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

LGTM

@siriak
Copy link
Member

siriak commented Jun 30, 2023

Don't forget to re-request a review once you are done with the changes :)

@lorenzotinfena
Copy link
Contributor Author

I tried to do little fixes on comments, and moved to a new folder called sqrt, it may contains in future others algorithms/data structures which use "sqrt advantages", for example mo's algorithm

@lorenzotinfena lorenzotinfena changed the title Added sqrt decomposition simple Added sqrt decomposition Jul 4, 2023
@lorenzotinfena
Copy link
Contributor Author

What should I do?

@siriak
Copy link
Member

siriak commented Jul 10, 2023

@tjgurwara99 could you review again?

@tjgurwara99
Copy link
Member

I haven't forgotten this PR - I will review as soon as I get some time 🙏🏼

Copy link
Member

@tjgurwara99 tjgurwara99 left a comment

Choose a reason for hiding this comment

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

Other than those two things, everything else looks good to me. You can also feel free to ignore it since it's an opinions related comment, in which case let me know and I'll approve it 😄

sqrt/sqrtdecomposition.go Show resolved Hide resolved
sqrt/sqrtdecomposition.go Show resolved Hide resolved
@lorenzotinfena
Copy link
Contributor Author

I did a little naming fix, but not the ones you exposed.
I'm not sure, but I think it make sense to interprete "sqrt" package as an abbreviation of something like "sqrt decomposition based algorithms and ds", and the base data structure (unfortunately) has the same name of the family of algorithms (sqrt decomposition) (or has no name: see https://cp-algorithms.com/data_structures/sqrt_decomposition.html). Can be included also in the same packages other sqrt-decomposition algorithms like mo's algorithm, so call it only Decomposition is not too intuitive to me.

FInally I think we have 2 choices: Let the name as it is, or call it like "BaseDS" (base data structure), what do you like more?

@lorenzotinfena
Copy link
Contributor Author

With all this effort in this conversation can be built tens of sqrt decompositions xd

Copy link
Member

@tjgurwara99 tjgurwara99 left a comment

Choose a reason for hiding this comment

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

The choice of naming is unfortunate and that can also be said to be true for quicksort hence we also made an exception there as well, so don't worry too much about naming 😄 It was just a suggestion because I rarely come across these sqrt decomposition based data structures so I know very little about them. My only concern was convention so I suggested it but otherwise it looks good.

@tjgurwara99 tjgurwara99 enabled auto-merge (squash) July 20, 2023 21:20
@tjgurwara99 tjgurwara99 merged commit 0b4b2de into TheAlgorithms:master Jul 21, 2023
2 checks passed
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.

5 participants