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

DM-29065: Sort all input lists by calexpList detector order in makeWarp #479

Merged
merged 1 commit into from Mar 11, 2021

Conversation

yalsayyad
Copy link
Contributor

No description provided.

Copy link
Contributor

@erykoff erykoff left a comment

Choose a reason for hiding this comment

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

This is fine, but I did get a bit confused about the overloaded term key in here.

else:
inputKeyOrder = [ref.datasetRef.dataId[dataIdKey] for ref in refs]
if inputKeyOrder != outputKeyOrder:
setattr(inputRefs, key, reorderAndPadList(refs, inputKeyOrder, outputKeyOrder))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be inputRefs[key] = reorderAndPadList(refs, inputKeyOrder, outputKeyOrder)?

Copy link
Contributor Author

@yalsayyad yalsayyad Mar 9, 2021

Choose a reason for hiding this comment

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

Nope.

(Pdb) inputRefs[key] 
*** TypeError: 'InputQuantizedConnection' object is not subscriptable

for key, refs in inputRefs:
if isinstance(refs, Iterable):
if hasattr(refs[0], "dataId"):
inputKeyOrder = [ref.dataId[dataIdKey] for ref in refs]
Copy link
Contributor

Choose a reason for hiding this comment

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

This also applies to the function above which has just been moved and therefore might be outside the scope of this ticket. But the term key is used in two different ways here. One is a dict-like key (as in for key, refs in inputRefs) which is very pythonic. The other is a sorting key which I guess python sort functions also use the term key in this way, but in this particular method I find the overloading of the term confusing.
It may be verbose, but maybe call inputKeyOrder -> inputSortKeyOrder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The plain key --> connectionName,
inputKeyOrder --> inputSortKeyOrder,
outputKeyOrder --> outputSortKeyOrder

@yalsayyad yalsayyad merged commit 309fef4 into master Mar 11, 2021
@yalsayyad yalsayyad deleted the tickets/DM-29065 branch March 11, 2021 18:47
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.

None yet

2 participants