Removed OrderedDict from io.ascii - #15496

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

• Do the proposed changes actually accomplish desired goals?
• Do the proposed changes follow the Astropy coding guidelines?
• Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
• Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
• Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
• Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
• Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
• Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
• Is a milestone set? Milestone must be set but we cannot check for it on Actions; do not let the green checkmark fool you.
• At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

Error is real, please fix.

Thanks!

How was this not caught by codestyle checks?

I think this might be a tricky one. It causes real failures in cosmology tests. I am turning this into draft for now.

pre-commit.ci autofix

Pinging @nstarman – for some reason this is causing the meta of cosmologies to be sorted alphabetically on reading back in (or already on writing), though for roundtripping ECSV directly everything passes. Something happening in cosmology._io.mapping?

Note: any time things are writting to yaml, dict ordering is lost. This is why we are still using an OrderedDict in meta. Almost certainly it is a similar problem here.

More generally: we did have a round of removing OrderedDict before - please find that PR and see the discussion there on why not everything was changed (sorry, too short on time myself to look for it, but hopefully it is easy to find it). This may also influence the PR for time.

So yaml is reordering a dict, but not OrderedDict?

And I have been wondering as well if e.g. #10888 (comment) might be relevant – sounds like something like this could be happening within mapping.

Yes, yaml dict does not preserve order. And, indeed, #10888 is the issue I was thinking of, with the PRs that link to it (including those closed without merging).

But yaml.dump has a sort_keys option. So presumably setting sort_keys=False in

However that would link this PR to a Remove OrderedDict from table task along with several dozen other instances in that module – or maybe not; the writer's default behaviour could possibly be changed without otherwise touching the use of OrderedDict?

Pinging @taldcroft as well as table maintainer – would that single change to serialising table headers make sense?

@dhomeier @alexandrecesarba - if we make the suggested change in meta.py, do tests pass?

pre-commit.ci autofix

OK, looks like

Could you try the latter change to see if that lets the tests pass?

There are some additional failures in the doctests now, but those actually show that the yaml.dump change actually works as expected – the docs just still expect an output that is reordered alphabetically.

However the real issue in cosmology.io seems unrelated to this, as that test is in fact going through the ECSV writer. Just realising though that it only fails for the _subclass_partial_info tests, and those are actually reading back a manipulated file that is opened and written back as a QTable. So I think we are back to having this linked to OrderedDict still being used in table (and I suspect it is not just QTable; notably TableColumns still is an OrderedDict subclass).

So I think this will still have to be synchronised with removing OrderedDict from table as well.

LMK what I can do to help on the cosmology side. Though if this is a Table issue, then perhaps the cosmology tests will "just work" when Table is addressed?

Would need @nstarman's input whether it makes sense to set sort_keys=False there as well.

Key order is important in cosmology ONLY for doctests.
I think having deterministic tests is somewhat important, so it would be good if the YAML dump preserved ordering of passed an OrderedDict, which implies the person cares about ordering.

Some logic like

if isinstance(sort_keys, bool):
    pass
elif sort_keys is None:
    sort_keys = True if isinstance(obj, OrderedDict) else False
else:
    raise TypeError("what are you doing?")

Key order is important in cosmology ONLY for doctests.

In general this only seems to be an issue with some doctests. So if we preserve order in the YAML output of dict in general, as I think we should (see below), just need to update the expected output in those docs.

Cosmology tests like *_readwrite_ecsv_* however do expect key order to be preserved as well, as checks like
assert got.meta == cosmo.meta will fail on changed key orders as long as the meta are OrderedDicts.
If both are standard dict, they will still qualify as equal, even though any given dict is preserving its key order.
So the issue may indeed resolve itself once everything is changed to dict (which supposedly means once Table.meta is becoming a regular dict, since cosmology is not explicitly calling OrderedDict anywhere – in particular as the one failing test group test_readwrite_ecsv_subclass_partial_info appears to be failing because of the intermediate QTable read/write operation).

I think having deterministic tests is somewhat important, so it would be good if the YAML dump preserved ordering of passed an OrderedDict, which implies the person cares about ordering.

Thought for a moment I misunderstood #15496 (comment), but that is already the default for OrderedDict, if you e.g. directly compare the output of yaml.dump(Cosmo.meta) and yaml.dump(dict(Cosmo.meta)).
But the ultimate point of #10888 is to replace OrderedDict everywhere, which implies that we should always care about ordering in dict as well. Thus my suggestion to have io.misc.yaml.dump preserve order by default.
Although we could still make this input-dependent along the lines of your suggestion as
sort_keys = False if isinstance(obj, (dict, OrderedDict)) else True
if there are really other objects where we want to keep re-sorting enabled.

But it looks like all of this really depends on getting table refactored as well.

@alexandrecesarba I started looking into this while going through #10888, do you still have the bandwidth to finish this up? I can cherry pick the commits and start a new PR if you aren't available right now :)

I started looking a bit more into this and we just need to do change the default factory in MetaData to dict to make cosmology tests happy. I will create a new PR which builds on top of this :)

--- a/astropy/utils/metadata/core.py
+++ b/astropy/utils/metadata/core.py
@@ -79,7 +79,7 @@ class MetaData:
         None
     """
 
-    def __init__(self, doc="", copy=True, *, default_factory=OrderedDict):
+    def __init__(self, doc="", copy=True, *, default_factory=dict):

Hi humans 👋 - this pull request hasn't had any new commits for approximately 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock.

If you believe I commented on this pull request incorrectly, please report this here.

@MridulS, @taldcroft, is #16250 superseding this PR as well?

I'm going to close this pull request as per my previous message. If you think what is being added/fixed here is still important, please remember to open an issue to keep track of it. Thanks!

If this is the first time I am commenting on this issue, or if you believe I closed this issue incorrectly, please report this here.