Skip to content

Fixing confusion for indexing python dictionaries#370

Closed
Benfeitas wants to merge 1 commit intodatacarpentry:gh-pagesfrom
Benfeitas:patch-1
Closed

Fixing confusion for indexing python dictionaries#370
Benfeitas wants to merge 1 commit intodatacarpentry:gh-pagesfrom
Benfeitas:patch-1

Conversation

@Benfeitas
Copy link
Copy Markdown
Contributor

@Benfeitas Benfeitas commented Apr 5, 2019

See #360

@Benfeitas Benfeitas changed the title Fixing possible confusion for python dictionaries Fixing confusion for indexing python dictionaries Apr 5, 2019
@pr4deepr
Copy link
Copy Markdown
Contributor

Not sure if there is a confusion actually.
The code:
translation = {'one': 1, 'two': 2} and
rev = {1: 'one', 2: 'two'}
illustrate that both strings (one and two) and numeric types (1 and 2) are acceptable as keys..

@Benfeitas
Copy link
Copy Markdown
Contributor Author

@pr4deepr I agree with you. However, learners may think that indexing may be done in terms of key position and key order is not guaranteed to be preserved - aside from insertion order. For the lesson at hand, I think that #360 (comment) makes sense and is simpler if we just have it as text.

@pr4deepr
Copy link
Copy Markdown
Contributor

Thanks @RBenRocks . Agree with that, but I think this bit should not be modified, but you can add these changes later on instead?
Towards the end of the 'dictionary' section, the issue you raise is addressed:
It is important to note that dictionaries are “unordered” and do not remember the sequence of their items (i.e. the order in which key:value pairs were added to the dictionary). Because of this, the order in which items are returned from loops over dictionaries might appear random and can even change with time.
But, I think adding a line of code to address this would be good as it is an important concept.

@maxim-belkin
Copy link
Copy Markdown
Contributor

I've been thinking about this PR and issue #360 and couldn't understand their relationship because the issue says:

When introducing a dictionary, numeric keys are used.

and this PR changes values and, moreover, the very first dictionary introduced does not actually use numeric keys. Now I understand that we're talking about the rev dictionary...

I support this change because we shouldn't be using numeric keys with dictionaries, actually:

a = {1 : '2'}
print(a)
a[1.0] = '3'
print(a)
a[True] = '4'
print(a)

And we should definitely fix that order in which key:value pairs were added to the dictionary part because it is preserved in Python since recently.

Also, statement You can think about a key as a name for or a unique identifier for a set of values in the dictionary. is not correct as there is one-to-one correspondence between keys and values, not one-to-many.

@pr4deepr
Copy link
Copy Markdown
Contributor

Thanks for clarifying this.
Didn't realise the order was conserved recently..

@maxim-belkin maxim-belkin requested a review from wrightaprilm May 17, 2019 14:41
>
> 1. First, print the value of the `rev` dictionary to the screen.
> 2. Reassign the value that corresponds to the key `2` so that it no longer
> 2. Reassign the value that corresponds to the key `second` so that it no longer
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In this new set of assignments, I do agree that using strings is a little more explicit. But I think we should make it clear that dictionaries are not only for strings. Perhaps we should have them reassign the value of second to the numeral 2 instead of apple-sauce.

@wrightaprilm
Copy link
Copy Markdown
Contributor

I like this. I've made one comment on the challenge. If you agree, go ahead and add that to this PR, if not just let me know.

> 1. First, print the value of the `rev` dictionary to the screen.
> 2. Reassign the value that corresponds to the key `2` so that it no longer
> 2. Reassign the value that corresponds to the key `second` so that it no longer
> reads "two" but instead "apple-sauce".
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
> reads "two" but instead "apple-sauce".
> reads "two" but instead "2".

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I went ahead and added my comment as a suggestion, since you seem on board, @RBenRocks. If you click the "Commit suggestion" box, it'll add my change to your PR. Then I'll merge and we're done!

@maxim-belkin
Copy link
Copy Markdown
Contributor

maxim-belkin commented Jan 28, 2020

A lot of things have changed since we discussed this PR.
I will revive it with April's suggestions.

@maxim-belkin
Copy link
Copy Markdown
Contributor

Superseded by #445

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants