Change Table and Column meta to dict and enforce YAML output as ordered map by taldcroft · Pull Request #16250 · astropy/astropy

taldcroft · 2024-03-29T18:38:58Z

Description

This builds on top of #16240 (which built from #15496) to work toward removing OrderedDict from astropy where possible. This PR does so in a number of places.

Table

Table.meta now defaults to dict.
Column.meta now defaults to dict.

It is important to remember that these attributes can still be set to any mapping type (e.g. t.meta = OrderedDict(...)), it is just the default type that is changed.

YAML / ECSV

The machinery which helps to serialize Table and Column to YAML (and back) has been updated to always write the Table and Column meta attribute as an ordered map (!!omap in YAML). This change addresses a previous inconsistency: if the user initialized a Table like t = Table(data, meta={'a': 1}) then the YAML would serialize meta as an unordered mapping. This was not the intent. The same held for Column.meta. Now the behavior is consistent and this is reflected in changes to unit tests and docstrings. However, the order of items in the dict is not changed.

This mostly just impacts ECSV. There should not be any issues with back-compatibility. The main change is just following the Table change, namely that reading an ECSV file will always return a Table that has dict meta instead of OrderedDict meta (except of course that past behavior was inconsistent).

Other `io.ascii` reader classes

These are somewhat unrelated to the more impacting changes but are useful nonetheless.

This PR should be squashed.

By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

cc: @MridulS @nstarman

github-actions · 2024-03-29T18:39:23Z

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

github-actions · 2024-03-29T18:39:29Z

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

nstarman · 2024-03-29T18:58:07Z

Cosmology changes LGTM. This is reversing the effects of #15484 which had updated meta to be a dict in the table, resulting in the change. This PR effects Table and thus Cosmology.

Thanks for the review. So this is making a little churn but should be OK.

mhvk

I like this approach! It puts the use of OrderedDict exactly where the problem lies, with yaml, and makes everything else easier. It is especially nice that order is now guaranteed to round-trip.

I assume it is on purpose that you do not (yet) change the default for meta itself?

taldcroft · 2024-04-04T13:41:55Z

I assume it is on purpose that you do not (yet) change the default for meta itself?

Yes, but that's a great question. I was thinking about making that change as a separate PR. And as usual when you start poking at some code you see little problems. E.g. for some reason Quantity info is not enforcing that meta should be dict-like:

In [2]: q = [1, 2] * u.m

In [3]: q.info.meta = "wrong"

In [4]: q.info.meta
Out[4]: 'wrong'

So anyway, maybe a follow-up PR that changes the default and fixes things like that?

mhvk · 2024-04-04T14:02:41Z

OK, let's just deal with getting YAML right here!

mhvk

Had a look through everything again and basically this looks all great to me. Probably good to have a changelog entry!

taldcroft · 2024-04-04T14:14:35Z

@mhvk - I've added changelog entries and a What's New. The expectation is that the What's New will be expanded to include the next PR to change the default to dict in MetaData itself.

AFAIK this PR is complete and ready for review. (Of course CI often disagrees...)

mhvk

Looks good! Main comments are to update a comment and docstring.

Also, the readthedocs failures may be related to 6.1 having been branched...:

/home/docs/checkouts/readthedocs.org/user_builds/astropy/checkouts/16250/docs/
index_getting_started.rst:7: WARNING: toctree contains reference to document 'whatsnew/6.1' that doesn't have a title: no link will be generated

mhvk · 2024-04-04T14:38:57Z

 class BaseColumn(_ColumnGetitemShim, np.ndarray):
-    meta = MetaData(default_factory=OrderedDict)
+    meta = MetaData(default_factory=dict)
    # It's important that the metadata is an OrderedDict so that the order of


Can you adjust this comment? Or perhaps just remove it?

mhvk · 2024-04-04T14:41:00Z

      OrderedDict([('foo', 'bar'), ('mumble', 'quux'), ('baz', 'gorp')])
    """
-    omap = OrderedDict()
+    omap = {}


I think the docstring needs to be adjusted here! Note that the doctest is skipped - I think that may be from a time when yaml was not a required dependency.

I ended up removing the example entirely. It turns out that it didn't work as shown. First there were no imports so there was ambiguity about the yaml symbol (from astropy or pyyaml?). But more importantly it didn't show that this private function needs to be registered as a constructor in a custom Loader subclass. The comment now just refers to the astropy function where it is used and that should be fine.

taldcroft · 2024-04-04T17:53:58Z

@mhvk - thanks for the quick review, your comments have been addressed. I just rebased again and with any luck the RTD will pass now.

taldcroft · 2024-04-04T17:55:11Z

And as a reminder I prefer that this be squashed.

mhvk · 2024-04-04T18:07:16Z

    License: Unspecified

    This is the same as SafeConstructor.construct_yaml_omap(),
    except the data type is changed to OrderedDict() and setitem is


Is this still true?

mhvk

I'll approve now, so you can merge when tests pass, but see my single comment.

taldcroft · 2024-04-08T10:21:17Z

@pllim @saimn @astrofrog - I'm still seeing an RTD warning /home/docs/checkouts/readthedocs.org/user_builds/astropy/checkouts/16250/docs/index_getting_started.rst:7: WARNING: toctree contains reference to document 'whatsnew/6.1' that doesn't have a title: no link will be generated which is causing the RTD build to fail.

I rebased on main on Friday. Any thoughts on fixing this?

saimn · 2024-04-09T10:16:30Z

Should be fixed with #16272.

nstarman

Cosmology looks G2G. Thanks!

This pull request removes OrderedDict from files in io.ascii. The files are: core.py , daophot.py , ecsv.py , fastbasic.py , ipac.py and test_read.py. The OrderedDicts are normal dictionaries now.

taldcroft · 2024-04-16T20:48:29Z

I rebased on main which will hopefully fix the RTD failure.

…ed map (#16250) * Use OrderedDict/omap sandwich in YAML * Removed OrderedDict from io.ascii This pull request removes OrderedDict from files in io.ascii. The files are: core.py , daophot.py , ecsv.py , fastbasic.py , ipac.py and test_read.py. The OrderedDicts are normal dictionaries now. * fixed indent error * Remove unused imports and ruff formatting * Use dict as default factory for meta instead of OrderedDict * Table and column meta default to dict, in YAML always !!omap * Make testing (esp ECSV) more robust for meta * Add change log entries * Add What's New entry * Minor doc updates --------- Co-authored-by: alexandrecesarba <[email protected]>

taldcroft added the API change PRs and issues that change an existing API, possibly requiring a deprecation period label Mar 29, 2024

taldcroft added this to the v7.0.0 milestone Mar 29, 2024

github-actions Bot added table io.ascii cosmology utils labels Mar 29, 2024

nstarman reviewed Mar 29, 2024

View reviewed changes

MridulS mentioned this pull request Mar 31, 2024

Remove ordereddict io.ascii #16240

Closed

1 task

mhvk reviewed Apr 1, 2024

View reviewed changes

mhvk reviewed Apr 4, 2024

View reviewed changes

taldcroft force-pushed the ascii-remove-ordereddict branch from 0f4753e to 626844a Compare April 4, 2024 14:12

taldcroft marked this pull request as ready for review April 4, 2024 14:12

taldcroft requested review from a team and dhomeier as code owners April 4, 2024 14:12

taldcroft requested a review from nstarman April 4, 2024 14:12

taldcroft added the whatsnew-needed label Apr 4, 2024

taldcroft requested a review from mhvk April 4, 2024 14:13

mhvk reviewed Apr 4, 2024

View reviewed changes

taldcroft force-pushed the ascii-remove-ordereddict branch from 626844a to ec0d4e1 Compare April 4, 2024 17:48

mhvk reviewed Apr 4, 2024

View reviewed changes

mhvk approved these changes Apr 4, 2024

View reviewed changes

dhomeier mentioned this pull request Apr 5, 2024

Removed OrderedDict from io.ascii #15496

Closed

nstarman approved these changes Apr 16, 2024

View reviewed changes

taldcroft and others added 10 commits April 16, 2024 16:45

Use OrderedDict/omap sandwich in YAML

b92ce91

Removed OrderedDict from io.ascii

e0d699a

This pull request removes OrderedDict from files in io.ascii. The files are: core.py , daophot.py , ecsv.py , fastbasic.py , ipac.py and test_read.py. The OrderedDicts are normal dictionaries now.

fixed indent error

f81b969

Remove unused imports and ruff formatting

f312767

Use dict as default factory for meta instead of OrderedDict

834a378

Table and column meta default to dict, in YAML always !!omap

2c57812

Make testing (esp ECSV) more robust for meta

bedd0c6

Add change log entries

57756a2

Add What's New entry

b82b15d

Minor doc updates

81b3c81

taldcroft force-pushed the ascii-remove-ordereddict branch from ec0d4e1 to 81b3c81 Compare April 16, 2024 20:47

taldcroft enabled auto-merge (squash) April 16, 2024 20:48

taldcroft merged commit d9ef612 into astropy:main Apr 16, 2024

taldcroft deleted the ascii-remove-ordereddict branch April 16, 2024 21:06

pllim mentioned this pull request Oct 30, 2024

TST: Use Python 3.12 or later for doctest in cosmology and table #17277

Merged

1 task

This was referenced Nov 5, 2024

Handle Metadata in io.ascii MRT Tables #17279

Draft

Update docs to reflect default Table.meta is now dict #17336

Merged

Conversation

taldcroft commented Mar 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Table

YAML / ECSV

Other io.ascii reader classes

Uh oh!

github-actions Bot commented Mar 29, 2024

Uh oh!

github-actions Bot commented Mar 29, 2024

Uh oh!

nstarman Mar 29, 2024

Choose a reason for hiding this comment

Uh oh!

taldcroft Apr 1, 2024

Choose a reason for hiding this comment

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

taldcroft commented Apr 4, 2024

Uh oh!

mhvk commented Apr 4, 2024

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

taldcroft commented Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk Apr 4, 2024

Choose a reason for hiding this comment

Uh oh!

taldcroft Apr 4, 2024

Choose a reason for hiding this comment

Uh oh!

mhvk Apr 4, 2024

Choose a reason for hiding this comment

Uh oh!

taldcroft Apr 4, 2024

Choose a reason for hiding this comment

Uh oh!

taldcroft commented Apr 4, 2024

Uh oh!

taldcroft commented Apr 4, 2024

Uh oh!

mhvk Apr 4, 2024

Choose a reason for hiding this comment

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

taldcroft commented Apr 8, 2024

Uh oh!

saimn commented Apr 9, 2024

Uh oh!

nstarman left a comment

Choose a reason for hiding this comment

Uh oh!

taldcroft commented Apr 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

taldcroft commented Mar 29, 2024 •

edited

Loading

Other `io.ascii` reader classes

taldcroft commented Apr 4, 2024 •

edited

Loading