Sam causes problems #56
Conversation
| Thank you very much for this, @HardBoiled800 . I will review this ASAP. I haven't been getting email notifications for this repo over the past couple of weeks! |
| @HardBoiled800 I just pushed revisions to this branch. Your write-up was great! I wouldn't have thought to place emphasis on the importance of copying the input dictionary as you did. The structure of the problem, clarity of presentation, and recommended extra challenges were all fantastic. The changes that I made were mainly motivated by using a slightly more succinct/readable code structure with explicit variable names. I also wanted to point out some additional 'gotchyas'. I also avoided the discussion of algorithm complexity - that discussion ended up being more nuanced once I really thought about it. The thing is, whether or not you use the optimization that you recommended, you do have to iterate over both dictionaries. When you make a copy of the dictionary, Python still needs to iterate over it under the hood. This process is indeed much faster than manually performing a for-loop, but it is hard to chalk this speedup to a change in algorithmic complexity. @davidmascharka can we get your professional editing skills on this? 😄 Once again, great work @HardBoiled800 ! |
| "Let's suppose that we have two [dictionaries](http://www.pythonlikeyoumeanit.com/Module2_EssentialsOfPython/DataStructures_II_Dictionaries.html#Data-Structures-(Part-II):-Dictionaries) that we want to merge together such that we always retain the *greater* value among mappings with common keys. Here's an example:\n", | ||
| "\n", | ||
| "```python\n", | ||
| "# merging two dictionaries, retaining the greatest values among\n", |
There was a problem hiding this comment.
greatest value; not values
| "# merging two dictionaries, retaining the greatest values among\n", | ||
| "# common keys\n", | ||
| ">>> dict1 = {'bananas':7, 'apples':3, 'pears':14}\n", | ||
| ">>> dict2 = {'bananas':3, 'apples':6, 'grapes':9}\n", |
There was a problem hiding this comment.
Can you add spaces between the colons and the values? It's ugly to read right now
| "{'bananas': 7, 'apples': 6, 'pears': 14, 'grapes': 9}\n", | ||
| "```\n", | ||
| "\n", | ||
| "Write a function that accepts two dictionaries and returns a dictionary with all of the mappings of the input dictionaries, but with the maximum value mapped to any duplicate values. Be sure to test this function!" |
There was a problem hiding this comment.
Phrasing here should be changed. You don't map values to keys; you map keys to values.
"..., but with the maximum value if there are duplicate keys" or something
| " return merged\n", | ||
| "```\n", | ||
| "\n", | ||
| "This function will correctly merge its two input dictionaries, however something insidious is afoot... Can you identify the bad behavior that will result from this function? Consider that dictionaries are [mutable](http://www.pythonlikeyoumeanit.com/Module2_EssentialsOfPython/Variables_and_Assignment.html#Referencing-a-Mutable-Object-with-Multiple-Variables) objects and that the statement `merged = dict1` simply assigns a variable that references `dict1` rather than creating a new copy of the dictionary.\n", |
There was a problem hiding this comment.
"...merge its two input dictionaries**;** however**,** something insidious..." <- replace the comma with a semicolon and add a comma after 'however' here.
| "\n", | ||
| "```python\n", | ||
| "a = {}\n", | ||
| "b = dict(zip(range(10000), range(10000))) # {1 : 1, 2: 2, ..., 9999:999}\n", |
There was a problem hiding this comment.
9999:9999 <-- you're missing a 9
| "metadata": {}, | ||
| "source": [ | ||
| "## Generalized Solution\n", | ||
| "Addressing point #1, we will will to use the `*args` syntax in our function signature so that [it can accommodate an arbitrary number of dictionaries](http://www.pythonlikeyoumeanit.com/Module2_EssentialsOfPython/Functions.html#Accommodating-an-Arbitrary-Number-of-Positional-Arguments). Thus all of the dictionaries fed to our function will be packed into a tuple that can be accessed via `args` (or whatever variable name we use in our signature).\n", |
There was a problem hiding this comment.
"...we will want to use..." not will will to use
| "While you may have thought to have returned `None` in the case of there being no dictionaries to merge, returning an empty dictionary is much better behavior as code downstream from our function will only need to accommodate one type of output. Additionally, our function's docstring promises that it will return a dictionary and we should always abide by this contract.\n", | ||
| "\n", | ||
| "### Handling One Input\n", | ||
| "In the case that our function is passed a single dictionary, our function effectively makes a (shallow) copy of that dictionary are returns it. Suppose we tried to be clever and wrote our function to pass a single dictionary through; e.g.\n", |
There was a problem hiding this comment.
"...copy of that dictionary and returns it" not "are returns it"
| " return merged\n", | ||
| "```\n", | ||
| "\n", | ||
| "What is wrong with this solution? The problem is similar to the one that we considered above; our function always returns a dictionary that is independent from its inputs, meaning that mutating the merged dictionary has no impact on any of the inputs dictionaries. This is no longer the case when we receive a single dictionary - here the \"merged\" dictionary is simply a reference of the input. Any subsequent mutation to the output dictionary will also mutate the input dictionary, and vice versa. This unanticipated behavior could create very hard-to-find bugs in your code!\n", |
There was a problem hiding this comment.
"...is a reference to the input" not "a reference of the input"
"...mutation of the output" not "mutation to the output"
| @HardBoiled800 this is all set! I will let you know when I post it to the site. How would you like to be credited on PLYMI? By name? GitHub handle? Twitter handle? A combo of these? |
| Wow! Thank you guys for making these edits - it looks great! @rsokl I'd love to be credited by name and GitHub handle! |
| @HardBoiled800 your content is live! :) |
3 participants
I added a problems.ipynb notebook to the Python folder, and added the merge_max_mappings problem that Ryan talked about in #55 as well as some bonus challenges for that problem and full solutions!
Also, just so you know, I undid that README change from the first commit.