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

is there still a need for irradiance functions to return OrderDict since Python-3.7 is minimum version? #1684

Open
mikofski opened this issue Mar 6, 2023 · 6 comments

Comments

@mikofski
Copy link
Member

mikofski commented Mar 6, 2023

Is your feature request related to a problem? Please describe.
Python-3.7 is the minimum python. I believe since Python-3ish, all dicts are ordered by default. So do we still need to use OrderedDict for returns in irradiance.py?

Describe the solution you'd like
If no longer necessary, remove import of OrderedDict and replace all calls with simple dictionary.

Describe alternatives you've considered
Leave it as is. It seems to function now, and there may be an added benefit b/c an OrderedDict can be compared to another mapping that doesn't respect order?

Additional context
related to #1455 and raised in #1179 (comment)

@mikofski mikofski mentioned this issue Mar 6, 2023
8 tasks
@AdamRJensen
Copy link
Member

This article describes a few reasons why one might want to continue using OrderedDicts:

1 - Intent signaling: If you use OrderedDict over dict, then your code makes it clear that the order of items in the dictionary is important. You’re clearly communicating that your code needs or relies on the order of items in the underlying dictionary.
2 - Control over the order of items: If you need to rearrange or reorder the items in a dictionary, then you can use .move_to_end() and also the enhanced variation of .popitem().
3 - Equality test behavior: If your code compares dictionaries for equality, and the order of items is important in that comparison, then OrderedDict is the right choice.
Backward compatibility Relying on regular dict objects to preserve the order of it

Though I still think that I favor the simplicity of getting rid of OrderedDicts.

@markcampanelli
Copy link
Contributor

I’d like to point out that one should never rely on the implementation of dict being ordered (as it is in newer CPython versions, IIRC for better performance). Thus, if a dictionary must be ordered for some good reason, then keep it declared as an OrderedDict.

@kandersolar
Copy link
Member

As I understand it, dicts being ordered according to insertion was a CPython implementation detail in 3.6 that got upgraded to an official language feature in 3.7 onward, so it should be reliable for our purposes :)

@markcampanelli
Copy link
Contributor

I read this part of the “tutorial” documentation for Python 3.12, and indeed it mentions that insertion order is guaranteed for certain operations. (Notably, when applied to a dict, sorted produces a generally different ordering of keys than list, which produces the insertion ordering.)

IMHO, this opens a can of worms about what contract you intend to uphold when supplying a dictionary, say, as a return value, because consumers now could apparently expect some consistent ordering, or perhaps not 🤷‍♂️. (It’s not uncommon for a dictionary to get constructed in a different order based on some internal logic.) Seems like a poor design choice for the language to make dict ordering more than an implementation detail, especially when there is still an OrderedDict. IMHO, it forces an element of complexity and risks making code and contracts more ambiguous.

https://docs.python.org/3/tutorial/datastructures.html#dictionaries

@adriesse
Copy link
Member

adriesse commented Dec 2, 2023

I just browsed some code that uses OrderedDict and I didn't see any reason why order even matters. Am I missing something?

@wholmgren
Copy link
Member

I started using OrderedDict because I wanted my plots to consistently use the same colors when iterating over dictionaries returned by pvlib. And I thought "well DataFrames have ordered columns so I guess in general dict-like pvlib results should too." I don't remember if there was ever another use case. That was nearly 10 years ago and a lot has changed since then!

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

No branches or pull requests

6 participants