You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Fixes the C++ example and accompanying text in Section 2.8 (Hash Table Analysis). The example previously measured the wrong thing and didn't match the surrounding prose or the figure.
Clarified that std::vector has no contains method; the example uses std::find for the vector and find/contains for the hash table.
Reworded the "same operation" description to accurately reflect what the C++ and Python listings each do.
Removed the claim about searching for random numbers (the code searches sequentially for every value in the container), matching what's actually implemented.
Timing correctness
The build/fill step for both the vector and hash table is now excluded from the timed region (only the search loop is measured).
The total elapsed time for each container's full search loop is now divided by a (container size) to report the average time per single search operation, rather than the cumulative time of searching every element. This is what the "Time to Complete Contains Operation" framing in the text (and prior figure) actually describes, and it produces the correct O(n) / O(1) trend instead of an O(n²) cumulative total for the vector case.
Confirmed via local runs (up to ~990,000 elements) that vector average search time grows linearly with size, and hash table average search time stays flat (~2 ns) regardless of size.
What's left to do
Figure (fig-vectvshash-cpp) — not included in this PR
The existing vectvshash.png reflects the old, incorrect cumulative-timing measurement (0–50 second scale) and no longer matches the corrected code's output (nanosecond/microsecond scale).
I've commented out the <figure> block and removed its <xref> from the prose so the doc builds cleanly without a misleading image.
A new chart needs to be generated from the corrected code's output (size vs. average vector/hash-table search time) and should replace vectvshash.png, after which the figure block can be uncommented and the xref restored.
I'm not sure how the original chart images in this book were generated — currently separatly working on trying to create a graph via Excel.
How this was verified
I compiled and ran the corrected code locally (g++, -O2 -std=c++17) across the full range (10,000 to 990,000 elements, step 20,000) to confirm the fix actually produces the expected trend before writing up the results in the text.
The values filled into the results paragraph were calculated directly from this output, not estimated.
This is the run that replaced the old cumulative-timing approach (which summed the time to search every element rather than averaging per-operation time, and produced a misleading result that didn't match the O(n)/O(1) claims in the text).
I like this, but the code takes advantage of auto. Is that added to our subset of the language at this point?
Note: I believe it probably should be, but we need to make sure to cover it when we discuss types (we need to be sure to demonstrate that auto doesn't imply dynamic typing, etc.)
@wrigjl Good catch. I missed that. I think that auto should be introduced, but it is not necessary here. What about explicitly using the actual data type here?
chrono::steady_clock::time_point begin = chrono::steady_clock::now();
time_point can be meaningful vocabulary, telling students that begin represents a point in time according to the steady clock.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes the C++ example and accompanying text in Section 2.8 (Hash Table Analysis). The example previously measured the wrong thing and didn't match the surrounding prose or the figure.
fixes #325
What's fixed
Documentation accuracy
std::vectorhas nocontainsmethod; the example usesstd::findfor the vector andfind/containsfor the hash table.Timing correctness
a(container size) to report the average time per single search operation, rather than the cumulative time of searching every element. This is what the "Time to Complete Contains Operation" framing in the text (and prior figure) actually describes, and it produces the correct O(n) / O(1) trend instead of an O(n²) cumulative total for the vector case.What's left to do
Figure (
fig-vectvshash-cpp) — not included in this PRvectvshash.pngreflects the old, incorrect cumulative-timing measurement (0–50 second scale) and no longer matches the corrected code's output (nanosecond/microsecond scale).<figure>block and removed its<xref>from the prose so the doc builds cleanly without a misleading image.vectvshash.png, after which the figure block can be uncommented and the xref restored.How this was verified
I compiled and ran the corrected code locally (g++,
-O2 -std=c++17) across the full range (10,000 to 990,000 elements, step 20,000) to confirm the fix actually produces the expected trend before writing up the results in the text.This is the run that replaced the old cumulative-timing approach (which summed the time to search every element rather than averaging per-operation time, and produced a misleading result that didn't match the O(n)/O(1) claims in the text).
Tested on local build
Reviewed by @harrisonj2-v