-
Notifications
You must be signed in to change notification settings - Fork 30
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
Moving from OrderedDict to dict for sat L2 readers #204
Comments
It's only official since 3.7. So maybe after #170 would be best.
Why is that? What in MM satellite wouldn't work if the reader returned a normal dict currently? Another option could be to just return a list of datasets instead, with the date key as a dataset attr. A dict could still be created from this on the fly in MM if necessary. |
Sounds good
The tempo tool has some checks that use isinstance(..., OrderedDict) to act differently if it's just a Dataset or a dictionary. (it only loops if it's an OrderedDict). This will have to get updated to isinstance(..., dict).
I like the idea of having a dict with the keys as timestamps, it makes it hashable and easy to access when, in the future, larger time periods are used. Although the fact that a list can be sliced is also nothing to scoff at... |
Feel free to assign me this, and I can slowly start moving towards it, without merging it until #170 is done. |
Note you could update that in MM anytime, since >>> isinstance(OrderedDict(), dict)
True |
Thanks! I was not aware of that. |
Since Python 3.6, dicts are order-preserving. Dicts have better performance than OrderedDicts, and therefore keeping the satellite readers as OrderedDicts seems unnecessary. Moving into dicts would also avoid having to import collections.
The performance difference in our case is negligible at best, but I think that it's still worth (slowly) pursuing and, at the very least, using dicts for future readers.
One concern is that this change has to be done simultaneously for MONETIO and MELODIES-MONET
The text was updated successfully, but these errors were encountered: