Skip to content

Fix serialization of structured masked arrays - #16380

Merged
mhvk merged 5 commits into
astropy:mainfrom
mhvk:table-serialize-masked-structured
May 5, 2024
Merged

Fix serialization of structured masked arrays#16380
mhvk merged 5 commits into
astropy:mainfrom
mhvk:table-serialize-masked-structured

Conversation

@mhvk

@mhvk mhvk commented May 3, 2024

Copy link
Copy Markdown
Contributor

Description

This pull request fixes multiple related failures for writing masked structured data to ECSV (either as MaskedColumn or Masked), which in turn meant some Time formats could not be serialized if the time was masked.

As a small side commit, it also ensures the repr of a MaskedRecarray shows the correct class name.

p.s. Not quite sure about the milestone. Put 6.1.1, but obviously nice if it can still get included in 6.1.0

Fixes #16370

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

@mhvk mhvk added this to the v6.1.1 milestone May 3, 2024
@mhvk
mhvk requested review from dhomeier and taldcroft as code owners May 3, 2024 20:26
@github-actions

github-actions Bot commented May 3, 2024

Copy link
Copy Markdown
Contributor

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.

@mhvk
mhvk force-pushed the table-serialize-masked-structured branch from 2b1071f to f4d84f8 Compare May 3, 2024 21:16
@mhvk mhvk mentioned this pull request May 3, 2024
Closed
@mhvk mhvk changed the title Table serialize masked structured Fix serialization of structured masked arrays May 4, 2024
taldcroft
taldcroft approved these changes May 5, 2024
View reviewed changes

@taldcroft taldcroft left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thanks @mhvk!

The only thing I noticed is that the repr of the test MaskedColumn in test_write_structured_masked_column is not what I would expect:

<MaskedColumn dtype='(int32, int32)' length=2>
    --  <== Should be (--, 2)
(3, 4)

I haven't looked into this. If there is a simple fix maybe it could be tossed in here, otherwise perhaps we need another issue?

@mhvk

mhvk commented May 5, 2024

Copy link
Copy Markdown
Contributor Author

Hmm, that is indeed annoying -- and something that must have been present for a long time! The repr goes through _pformat_col and presumably it is the default formatter that needs adjusting. My sense is that a separate issue is probably better (this one already combines a bit more than perhaps reasonable...), so let me just merge this.

@mhvk
mhvk merged commit 3f188e3 into astropy:main May 5, 2024
meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request May 5, 2024
@mhvk @meeseeksmachine
Backport PR astropy#16380: Fix serialization of structured masked arrays
eddb0fa
@meeseeksmachine meeseeksmachine mentioned this pull request May 5, 2024
Merged
@mhvk mhvk mentioned this pull request May 5, 2024
Open
@mhvk

mhvk commented May 5, 2024

Copy link
Copy Markdown
Contributor Author

See #16387 for the issue with MaskedColumn.__repr__.

@mhvk
mhvk deleted the table-serialize-masked-structured branch May 5, 2024 13:18
mhvk added a commit that referenced this pull request May 5, 2024
@mhvk
Merge pull request #16386 from meeseeksmachine/auto-backport-of-pr-16…
1e2a536 
…380-on-v6.1.x

Backport PR #16380 on branch v6.1.x (Fix serialization of structured masked arrays)
mhvk added a commit to mhvk/astropy that referenced this pull request May 6, 2024
@mhvk
Fix endian-dependence of test introduced in astropygh-16380
97eb6b2
@mhvk mhvk mentioned this pull request May 6, 2024
Merged
1 task
@mexanick mexanick mentioned this pull request May 6, 2024
Closed
pllim added a commit that referenced this pull request May 6, 2024
@pllim
Merge pull request #16391 from mhvk/utils-masked-pr-fix
8c425b8 
Fix endian-dependence of test introduced in gh-16380
meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request May 6, 2024
@pllim @meeseeksmachine
Backport PR astropy#16391: Fix endian-dependence of test introduced in …
85d6d47 
…astropygh-16380
@meeseeksmachine meeseeksmachine mentioned this pull request May 6, 2024
Merged
pllim added a commit that referenced this pull request May 7, 2024
@pllim
Merge pull request #16397 from meeseeksmachine/auto-backport-of-pr-16…
29dab0b 
…391-on-v6.1.x

Backport PR #16391 on branch v6.1.x (Fix endian-dependence of test introduced in gh-16380)
d-giles referenced this pull request in d-giles/astropy-abdufork Jul 26, 2024
@mhvk @d-giles
Fix endian-dependence of test introduced in gh-16380
fff9802
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@taldcroft taldcroft taldcroft approved these changes
@dhomeier dhomeier Awaiting requested review from dhomeier dhomeier is a code owner

Assignees

No one assigned

Labels

Bug io.ascii table time utils.masked utils

Projects

None yet

Milestone

v6.1.1

Development

Successfully merging this pull request may close these issues.

TypeError on write when stacking an empty table with a table that has a Time column

2 participants

@mhvk @taldcroft