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

Use dict instead of OrderedDict #6566

Closed
yarikoptic opened this issue Mar 18, 2022 · 4 comments · Fixed by #7174
Closed

Use dict instead of OrderedDict #6566

yarikoptic opened this issue Mar 18, 2022 · 4 comments · Fixed by #7174
Labels
performance Improve performance of an existing feature

Comments

@yarikoptic
Copy link
Member

dict is ordered now in versions of python we support. And according to https://realpython.com/python-ordereddict/ - dict seems to generally outperform OrderedDict in CPU and memory, and I don't think we use any of specific to OrderedDict functionality.

(git)lena:~datalad/datalad-maint[bf-sve]git
$> git grep OrderedDict | wc -l 
64

I think it would not hurt to just switch to just use dict.

@adswa
Copy link
Member

adswa commented Nov 11, 2022

I believe I found one specific use of OrderedDict, namely popitem(last=False), used in status:

datalad/core/local/status.py:635:        qdspath, qpaths = paths_by_ds.popitem(last=False)

The standard dict's popitem method doesn't take arguments, and returns items in LIFO order, but the last=False parameter in OrderedDicts results in FIFO order.

@yarikoptic
Copy link
Member Author

good catch! popitem(last=False) could be replaced with some helper like

k = next(iter(d.keys())); v = d.pop(k)

I guess. The key would be the first one. If a singular use, could be just applied as above without carrying to create a helper.

@adswa
Copy link
Member

adswa commented Nov 15, 2022

done in #7174

@adswa adswa closed this as completed Nov 15, 2022
@yarikoptic-gitmate
Copy link
Collaborator

Issue fixed in 0.18.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Improve performance of an existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants