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

Tickets/dm 11653 #70

Merged
merged 4 commits into from Oct 3, 2017
Merged

Tickets/dm 11653 #70

merged 4 commits into from Oct 3, 2017

Conversation

mfisherlevine
Copy link
Contributor

No description provided.

@@ -110,7 +112,7 @@ def _makeAmpInfoCatalog(self, ccd):
"""
# Much of this will need to be filled in when we know it.
assert len(ccd['amplifiers']) > 0
amp = ccd['amplifiers'].values()[0]
amp = list(ccd['amplifiers'].values())[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont know how big the ccd dictionary is, but most of the time you want to avoid creating a whole list object just to get the first element, as that can be expensive and will be thrown away right away. Something like:
amp = next(iter(ccd['amplifiers'].values())) would be better. However be sure that this is how you actually want to grab the amp. If this is an unordered dict, if someone changes something in the future 0 might not be the correct index. I dont know specifically what you are trying to do here, so this is just more of a general knowledge comment

Copy link
Member

Choose a reason for hiding this comment

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

The iter won't be needed on python3 so it's one of those code cleanups we'll have to look out for next spring when we drop python2.

Copy link
Member

Choose a reason for hiding this comment

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

Who added the list? Is it needed, or is it someone worrying about changes in iterators in py2/py3 and we can just use ccd['amplifiers'].values()[0] now and forever?

Copy link
Member

Choose a reason for hiding this comment

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

You can't index into values() on python3 because it's an iterator. You need to turn it into a list if you want to select specific entries.

Copy link
Contributor

@natelust natelust Oct 3, 2017

Choose a reason for hiding this comment

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

Tim, values() of a dictionary is not a generator, it is a dictionary_values object which is just a dictionary view, which is why the iter is necessary even on python 3

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. I always miss that dict_values() thing.

>>> d = {1: 2, 3: 4, 5: 6}
>>> d.values()
dict_values([2, 4, 6])
>>> d.values()[0]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'dict_values' object does not support indexing
>>> next(d.values())
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'dict_values' object is not an iterator
>>> next(iter(d.values()))
2

Copy link
Member

Choose a reason for hiding this comment

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

I knew it was a generator but assumed that they'd added the syntactic sugar to make this work.

The code doesn't care that it's the first amp, it just wants one so ordering is unimportant

Copy link
Contributor

Choose a reason for hiding this comment

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

Values gives you an object that will stay up to date if someone modifies the original dictionary later, which can be useful. However in this case it seems that Merline only wants the first value for whatever definition of first he is choosing (if this is not an ordered dict).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given the length of this list will likely never be over 16 elements, I will leave as-is for clarity, but I take the point for future uses if/when large lists might be in use.

@mfisherlevine mfisherlevine merged commit 449dd66 into master Oct 3, 2017
@ktlim ktlim deleted the tickets/DM-11653 branch August 25, 2018 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants