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

Moving from OrderedDict to dict for sat L2 readers #204

Open
blychs opened this issue Oct 8, 2024 · 5 comments
Open

Moving from OrderedDict to dict for sat L2 readers #204

blychs opened this issue Oct 8, 2024 · 5 comments
Assignees

Comments

@blychs
Copy link
Contributor

blychs commented Oct 8, 2024

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

@zmoon zmoon changed the title Moving from OrderedDict into dict Moving from OrderedDict to dict for sat L2 reader Oct 8, 2024
@zmoon zmoon changed the title Moving from OrderedDict to dict for sat L2 reader Moving from OrderedDict to dict for sat L2 readers Oct 8, 2024
@zmoon
Copy link
Member

zmoon commented Oct 8, 2024

Since Python 3.6, dicts are order-preserving.

It's only official since 3.7. So maybe after #170 would be best.

One concern is that this change has to be done simultaneously for MONETIO and MELODIES-MONET

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.

@blychs
Copy link
Contributor Author

blychs commented Oct 8, 2024

Since Python 3.6, dicts are order-preserving.

It's only official since 3.7. So maybe after #170 would be best.

Sounds good

One concern is that this change has to be done simultaneously for MONETIO and MELODIES-MONET

Why is that? What in MM satellite wouldn't work if the reader returned a normal dict currently?

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).

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.

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...

@blychs
Copy link
Contributor Author

blychs commented Oct 8, 2024

Feel free to assign me this, and I can slowly start moving towards it, without merging it until #170 is done.

@zmoon
Copy link
Member

zmoon commented Oct 8, 2024

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).

Note you could update that in MM anytime, since

>>> isinstance(OrderedDict(), dict)
True

@blychs
Copy link
Contributor Author

blychs commented Oct 8, 2024

Thanks! I was not aware of that.

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

2 participants