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

Segment tree enhancements #56

Merged
merged 14 commits into from
Nov 22, 2015

Conversation

mdclyburn
Copy link
Contributor

The reference code for the segment tree can now easily support other operations. I've manually tested all three implementations that are there. I'm also looking into writing a test for all three that are there at the same time. I only have to figure how that would work. The code here is up for review. Any feedback to make this more readable is more than welcome. I didn't make the SegmentTreeType an argument to the functions because there is already a sizable amount of arguments to at least one of the functions. I can see that this may be a problem if two segment trees of different types are needed though. This is changeable, but I would like to know if this should perhaps be the default?

This is also for another round of reviewing the sample problem included with the section. I aimed for a clear input format that has no extra twists to it. This makes it simpler to understand as well. Pinging @bcdean, @robertu94, and perhaps even @ProtractorNinja since I don't recall you ever seeing the sample problem.

Also, does anyone know of a real contest problem that called for a segment tree? That would be nice to include in this section as well.

Takes care of issue #45.

The reference code for the segment tree can now store different
types of data, depending on what is specified as the type in the
SegmentTreeType variable 'type'.

Using the helper function in segment tree operations automatically
handles returning the appropriate data to store in the nodes of
the tree.
@bcdean
Copy link

bcdean commented Nov 15, 2015

In terms of contest problems, I get quite a few hits when searching Google for “segment tree site:usaco.org”.

“To properly represent tree” -> “To properly represent the tree”

Maybe in the initial paragraph make it clear that prefix sum precalculation works for range sums but not range mins or maxes.

I like the new figure, but I find it confusing reconciling it with figure 1.1, and in particular I find the “empty spaces” in the leaves of the segment tree a bit confusing when the bottom of the tree is mapped to a contiguous array as in the example. Normally, if the leaves of the segment tree map to an array, the leaves would be completely filled in by the contents of the array in a contiguous fashion instead of leaving holes like this.

I think the problem is easier to read now. Maybe someone else wants to have a look as well though just to make sure it’s clear on first impression (since now I know what the problem is supposed to be asking).

Best,
-- Brian

From: Marshall Clyburn [mailto:[email protected]]
Sent: Sunday, November 15, 2015 12:48 PM
To: clemsonacm/hackpack
Cc: Brian Dean
Subject: [hackpack] Segment tree enhancements (#56)

The reference code for the segment tree can now easily support other operations. I've manually tested all three implementations that are there. I'm also looking into writing a test for all three that are there at the same time. I only have to figure how that would work. The code here is up for review. Any feedback to make this more readable is more than welcome. I didn't make the SegmentTreeType an argument to the functions because there is already a sizable amount of arguments to at least one of the functions. I can see that this may be a problem if two segment trees of different types are needed though. This is changeable, but I would like to know if this should perhaps be the default?

This is also for another round of reviewing the sample problem included with the section. I aimed for a clear input format that has no extra twists to it. This makes it simpler to understand as well. Pinging @bcdeanhttps://github.com/bcdean, @robertu94https://github.com/robertu94, and perhaps even @ProtractorNinjahttps://github.com/ProtractorNinja since I don't recall you ever seeing the sample problem.

Also, does anyone know of a real contest problem that called for a segment tree? That would be nice to include in this section as well.

Takes care of issue #45#45.


You can view, comment on, or merge this pull request online at:

#56

Commit Summary

  • Make segment tree multifunctional.
  • Move function documentation inside block.
  • Correct array visual.
  • Remove blank line.

File Changes

Patch Links:


Reply to this email directly or view it on GitHubhttps://github.com//pull/56.

@mdclyburn
Copy link
Contributor Author

@bcdean Do you think that it would be better that I left out the empty nodes so that the leaves are the exact same as the array? I avoided using a full array to better show the flexibility of the data structure, but maybe that's not necessary?

Thanks for the suggestions.

@bcdean
Copy link

bcdean commented Nov 15, 2015

Probably best to do this. I agree it’s nice to show the flexibility of the structure, but if this were to be done, you might want to use a different example than one where the leaves are considered to be mapped to an array.

-- Brian

From: Marshall Clyburn [mailto:[email protected]]
Sent: Sunday, November 15, 2015 5:21 PM
To: clemsonacm/hackpack
Cc: Brian Dean
Subject: Re: [hackpack] Segment tree enhancements (#56)

@bcdeanhttps://github.com/bcdean Do you think that it would be better that I left out the empty nodes so that the leaves are the exact same as the array? I avoided using a full array to better show the flexibility of the data structure, but maybe that's not necessary?

Thanks for the suggestions.


Reply to this email directly or view it on GitHubhttps://github.com//pull/56#issuecomment-156864060.

void update_segment_tree(int* const tree, const unsigned int start, const unsigned int end, const unsigned int changed_idx, const int new_val, const unsigned int st_idx)
{
// #ifdef hackpackpp
// update a value in the segment tree
//
// To support range sums, this function would have to be modified to recurse
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't strictly relevant to the solving of the problem. Instead of this long comment here, just include segment-tree.cpp in a section called "Reference" and put this comment there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm removing it. I didn't realize that this was irrelevant when I was going through it. Since the reference code easily does this now, it shouldn't be here any more.

@robertu94
Copy link
Contributor

As for including the type of the segment tree, I would group the functions into a SegmentTree class and make the type an argument of the constructor. I know that this is a tad heavy for contest style, but we can extract the correct implementation fairly easy given that you provide all three in a clear fashion. However, it does allow you to internalize some globals and initialize several trees which may save you lines of code in and fewer parameters to boot.

You could even include it as a default argument in c++98 which we see at ACM regional for whatever reason like so

class SegmentTree{
    public:
    unsigned int N;
    int * data;
    SegmentTree(int size,SegmentTreeType tree_type=ST_MIN){/* etc */}
    /* etc */
};

@robertu94
Copy link
Contributor

I still think the input format section could be a tad clearer that each of the C blocks refers to a specific cow, for example:

%  ...
    \item The following lines are placed in $C$ groups detailed as follows:
    \begin{itemize}
        \item A single line containing a single integer, $k$, representing the number of time ranges to follow that a single cow was present in the barn.

        \item $k$ lines with two distinct integers representing times between which that cow was present in the barn.
        The first number is guaranteed to be strictly less than the second number.
        \item A single line containing two integers $t_1$ and $t_2$ representing times between which the maximum number of cows that have been entered so far that could seen in the barn simultaneously
%...

The hint on changing the function to support range sums is no
longer important since this is easily done by the segment tree
provided in the reference code.
The class version is a bit more heavy-weight than the set of
functions, but this should support multiple segment trees of
varying types at once.
@mdclyburn
Copy link
Contributor Author

Anyone else have feedback for this? If not, I'd like to get this merged in.

@robertu94
Copy link
Contributor

The problem looks good at this point; however, the reference code should also be in the hackpack++ version. After that is fixed, it looks good to me.

@mdclyburn mdclyburn merged commit 3200969 into clemsonacm:master Nov 22, 2015
@mdclyburn mdclyburn deleted the segment-tree-enhancements branch November 22, 2015 02:08
@ProtractorNinja
Copy link
Contributor

Oh. Good. Yes. I was just thinking about looking at this. Let this be a lesson to me.

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.

4 participants