Avoid implicit bytes/text coercion in tables and metadata by ngoldbaum · Pull Request #20599 · astropy/astropy

ngoldbaum · 2026-10-14T00:10:04Z

Description

This pull request addresses implicit NumPy bytes/text conversions in astropy.table and astropy.utils.metadata, exposed by the pending NumPy PR numpy/numpy#32757. I have a draft NumPy change that deprecates bytes/text promotion and safe/same-kind casts and adds a new NPY_COERCION_POLICY=strict mode for downstream testing. I used it to check astropy and found some issues.

Here is how Astropy main and this PR compare (strict is the policy from numpy/numpy#32757):

import numpy as np
import pandas as pd
from astropy.table import MaskedColumn, Table

column = MaskedColumn([b"\xff", "a"], dtype=object)
# main, released NumPy: UnicodeDecodeError
# main, strict policy: DTypePromotionError
# This PR:
column.tolist()        # [b"\xff", "a"]
column.mask.tolist()   # [False, False]

frame = pd.DataFrame({"name": ["M31", np.nan]}, dtype=object)
result = Table.from_pandas(frame)
# main, strict policy: DTypePromotionError
# This PR:
result["name"].dtype          # dtype('<U3')
result["name"].mask.tolist()  # [False, True]

I've made the following changes in Astropy:

Table.from_pandas and Table.from_df use a matching bytes or text placeholder for missing values. This also fixes the pandas path used by ECSV reading.
Byte-column operations choose a dtype explicitly for flat lists and tuples, preserving existing conversions and errors. Bytes-only sequences skip an unnecessary array conversion. Nested mixed bytes/text inputs still depend on NumPy's promotion rules.
Joins and stacks choose the common Unicode width explicitly. Joins retain their existing matching rules and ASCII conversions; no new UTF-8 decoding is introduced.
Metadata arrays are concatenated with an explicit dtype and unsafe casting. This intentionally opts into NumPy's legacy bytes/text coercion, preventing promotion errors from becoming metadata conflicts that discard the left-hand values. Conversion failures still follow the existing metadata_conflicts setting.

I also benchmarked and optimized these paths against main. With released NumPy and 100,000 elements, dataframe imports with missing strings take 24–28% less time. Byte-column construction from a list of bytes improves from 8.86 to 2.80 ms, assignment from 13.94 to 7.94 ms, and comparison from 13.47 to 7.49 ms. Text-list performance is essentially unchanged. I didn't identify any significant regressions attributable to this patch in the Astropy benchmarks I investigated or the targeted synthetic benchmarks.

AI Disclosure

OpenAI Codex (GPT-6 Astra) assisted with the implementation, tests, changelog entries, downstream investigation, performance measurements, and this description. Those portions include content produced in a session iterating with an AI model. Additional AI review used Claude Opus 5.5.

I certify that I am human and that I take full responsibility for this pull request including all interactions with reviewers.

Merge method

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.

giles · 2026-10-14T00:10:15Z

Hi 👋 and thank you for your contribution! 🙏

This pull request is being closed automatically - but don't worry, this is not the end, and we may re-open it.

For some background, we have recently started seeing a rapid increase in the number of pull requests opened. Some of these are from good-faith humans, but some of which are from autonomous LLM agents or humans using LLMs who do not have a genuine understanding of, or interest in, the project. We are therefore auto-closing pull requests from new contributors, but if you are a good-faith human, we want to make sure your pull request gets considered! So if you would like us to re-open your pull request so that it gets reviewed, you need to do two things:

Add a comment here to explain why you need the changes here to be considered, as in how the bug or missing feature affects your work, or whether this is an issue you have encountered but does not affect you.
Join the astropy slack using this invite link and head over to the #new-contributor-requests channel to introduce yourself and let us know about this pull request

The second step is important, as we may otherwise miss notifications about this pull request.

To be clear, our AI policy does allow the use of LLMs as part of contributions, but we need to see authentic engagement and understanding from humans making the contribution.

Notes for maintainers

If you know this is a genuine user and the PR is is compliance with the AI policy, you may re-open this straight away. After two successful merged pull requests, the user will be added to the allowlist and future PRs will no longer be closed automatically.

This user has not made any pull requests to this repository prior to this one.

In addition, here are some statistics on the user's activity on GitHub:

	Last 24 hours	Last 7 days
Pull requests opened	2	6
Issues opened	1	1

github-actions · 2026-10-14T00:10:19Z

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.

ngoldbaum · 2026-10-14T00:10:25Z

Ping @mhvk as well

mhvk · 2026-10-14T00:26:48Z

Hmm, funny that the first encounter I have with the auto-close bot is you...

mhvk · 2026-10-14T00:29:06Z

ping @taldcroft, as it is more up your alley. This is part of trying to make casting between bytes and strings more logical (starting with a deprecation).

taldcroft · 2026-10-14T10:38:22Z

@ngoldbaum - thanks! This looks generally good but I'll need some time to get through it.

The one thing that you can address now are getting proper towncrier changelog fragments for the changes instead of the text-type-bugfix.rst files. Those changelogs should be first aimed at users and describe changes that might affect the behavior of existing code (and not so much the implementation details). This includes the obvious bugfix one of not raising UnicodeDecodeError as highlighted in the description. But are there are other cases where the result dtype might change?

Also, are there any performance impacts?

pllim

We usually backport numpy compatibility changes, but will this be too disruptive for v8.0.x or v7.2.x ? Max-pin numpy on them is also an option if we choose not to backport. Hope you all can advise. Thanks!

pllim · 2026-10-14T14:01:39Z

You need to rename this fragment as per https://github.com/astropy/astropy/blob/main/docs/changes/README.rst

pllim · 2026-10-14T14:01:44Z

ngoldbaum · 2026-10-14T14:10:48Z

We usually backport numpy compatibility changes, but will this be too disruptive for v8.0.x or v7.2.x ? Max-pin numpy on them is also an option if we choose not to backport. Hope you all can advise.

I'm not planning to change any behavior in NumPy, except optionally, via an env variable, for testing. So I'd say "do nothing" and let old astropy releases trigger deprecation warnings in new NumPy in a few spots is fine. The warning will also only trigger if someone really is mixing strings and bytes, which they should be able to work around by explicitly encoding or decoding data.

If you're really nervous I could make the NumPy warning be a PendingDeprecationWarning, but basically no one sees those.

pllim · 2026-10-14T15:21:41Z

I could make the NumPy warning be a PendingDeprecationWarning, but basically no one sees those

I agree that is not very useful. NumPy should do what it thinks is sensible and we can adapt here one way or another. I guess it was more of a question or @taldcroft or @mhvk in terms of astropy maintenance itself. :)

pllim · 2026-10-14T15:43:32Z

Also, are there any performance impacts?

I applied the benchmark label though the benchmark job is testing against stable numpy so YMMV.

ngoldbaum · 2026-10-14T22:12:08Z

I've updated the changelogs to cover an existing MaskedColumn bug that this incidentally fixes. I removed the unnecessary compatibility note. I also added two performance notes about performance improvements in the latest version.

The patch now preserves existing encoding behavior, dtypes, and join matching rather than assuming UTF-8 encoding.

The benchmark run on CI I think shows noise in the Quantity benchmarks. However some targeted microbenchmarks did identify some slowdowns. I found opportunities to use some compiled helpers to buy back some performance. Local benchmarks against main show faster dataframe imports and byte-column operations enabled by the new Cython helper. I now think there are no performance regressions.

ngoldbaum · 2026-10-15T16:25:25Z

+
+
+def _get_string_dtype(value):
+    """Identify flat bytes-only or Unicode sequences without promotion."""


I could deal with nested sequences too but thought those were too niche for the additional complexity.

how much additional complexity would it take to just detect them and emit a clear NotImplementedError ?

We'd have to add the additional complexity (~25 lines of code here) to detect them and raise the error. Also nested all-bytes or all-strings sequences are fine and need to continue to work, the only possible issue is nested sequences that mix bytes and strings, which I think are a pretty niche thing that aren't worth that extra complexity to detect.

Fair enough. Thanks !

pllim · 2026-10-15T21:26:52Z

devdeps job is green now, so I think this is ready for @taldcroft to re-review. 🙏

neutrinoceros

Thank you Nathan, this looks amazing. I have a couple, hopefully minor questions.

neutrinoceros · 2026-10-20T09:59:22Z

            # FITS does not perfectly preserve dtype: byte order can change, and
            # unicode gets stored as bytes.  So, we just check safe casting, to
            # ensure we do not, e.g., accidentally change integer to float, etc.
+            if a1.names is not None:


Note to self there are actually 5 versions of this assert function, only 2 of which are edited in this PR. I checked that the change doesn't make sense for the other 3.

I have a draft PR trying to unify them that's been blocked for a while, but this gives motivation to revive it.

neutrinoceros · 2026-10-20T10:01:27Z

+    @pytest.mark.parametrize("dtype", [None, "S", "U", object])
+    def test_masked_multidim_as_list(self, dtype):
+        data = np.ma.MaskedArray([1, 2], mask=[True, False], dtype=dtype)
+        c = table.MaskedColumn([data], dtype=dtype)


shouldn't we check that dtype is implicitly conserved instead of forcing it explicitly here ?

I'll expand this test a bit to test table.MaskedColumn([data]) as well.

neutrinoceros · 2026-10-20T10:08:23Z

I read the existing conversation about not backporting the entire PR, which seems sensible, but I want to ask wether it'd be possible to backport just this part ?

I think so.

neutrinoceros · 2026-10-20T10:10:10Z

+
+
+def _get_string_dtype(value):
+    """Identify flat bytes-only or Unicode sequences without promotion."""


how much additional complexity would it take to just detect them and emit a clear NotImplementedError ?

neutrinoceros · 2026-10-20T10:11:15Z

+            not isinstance(item, (int, float, complex, np.number))
+            and not isinstance(item, np.bool_)


I don't understand the point of making two calls to isinstance here when one would suffice. Am I missing anything ?

you're right, I will consolidate these.

neutrinoceros · 2026-10-20T10:14:20Z

+            if fill is None:
+                fill = "" if isinstance(value, str) else b""
+        elif legacy_pandas:
+            if value is not nan:


I didn't know that numpy.nan was a singleton ! Just to make sure I understand this correctly, does numpy only use a single NaN value across multiple dtypes ?

it's not a singleton:

>>> np.nan is float(float('nan')) False

However, this is a pre-existing bug in this code path:

astropy/astropy/table/_dataframes.py

Lines 531 to 533 in 0bfd49c

string_types = (str, bytes)

nan = np.nan

if all(isinstance(x, string_types) or x is nan for x in data):

Do you want me to address this issue here? Or is just a followup issue ok?

I see. I think it's okay to address here. table._dataframes is very new so I don't think we need an opt-in bug-for-bug legacy path at all.

the diff is kinda big and I don't want to hold off merging this on reviewing another big chunk of code, so I'm going to open an issue I think.

Yes, I'd like to get this merged basically like it is.

taldcroft · 2026-10-20T13:39:49Z

@ngoldbaum - can you use Claude to add more complete NumpyDoc-style docstrings and add some code comments? This is my preference for table code (especially now that it is so easy to generate). This will help me and future maintainers read the code.

ngoldbaum · 2026-10-20T13:55:48Z

can you use Claude to add more complete NumpyDoc-style docstrings and add some code comments? This is my preference for table code (especially now that it is so easy to generate). This will help me and future maintainers read the code.

Happy to do that. It's not my personal preference: I find it a lot harder to read heavily commented code.

neutrinoceros · 2026-10-20T14:02:18Z

I find it a lot harder to read heavily commented code.

this has been my opinion too for well before LLMs were a thing, but if their popularity actively incites devs to add comments that no-one wants to write, it's going to get even harder to want to read them...
Please remember: comments do not replace clear code. 'Generating' them was never the hard part. It's making sure they make sense and continue to make sense as the code evolves around them. LMMs cannot do that, unless you require every dev to use them (ouch), and if you do, you're also half-admitting no-one actually wants to read this text noobody cared to write.

taldcroft · 2026-10-20T14:04:04Z

Happy to do that. It's not my personal preference: I find it a lot harder to read heavily commented code.

Yeah, I noticed that in another numpy PR. I'm just the opposite. 😄

taldcroft · 2026-10-20T14:57:06Z

@ngoldbaum - I've reviewed this and understood all the bits and it looks basically good.

The only thing are the docstrings and comments, and the comment from @neutrinoceros about the separate isinstance(bool). You can tell Claude to add "concise comments". I'm not too proud to admit that this PR was not easy for me to understand because there are a lot of cross-code and cross-package interactions and behaviors, and the comments will certainly be helpful to me in the future. I can't imagine that anyone would argue against correct and complete docstrings.

ngoldbaum · 2026-10-20T16:32:28Z

The comments are more concise now, let me know if you'd like another pass on them.

taldcroft · 2026-10-20T16:51:01Z

@ngoldbaum - it looks like you merged main into this branch, which is not the "astropy way". Can you remove
f7815dc and if there is a merge conflict then rebase on main. I'm fine with the rebase if it happens.

ngoldbaum · 2026-10-20T17:03:47Z

Sorry! Thanks for dealing with me fumbling along...

taldcroft

Thanks @ngoldbaum, great work!

ngoldbaum requested review from WilliamJamieson, matteobachetti, neutrinoceros, saimn and taldcroft as code owners October 14, 2026 00:10

giles Bot closed this Oct 14, 2026

github-actions Bot added table io.fits utils io.misc labels Oct 14, 2026

mhvk reopened this Oct 14, 2026

pllim added this to the v8.1.0 milestone Oct 14, 2026

pllim added the numpy-dev label Oct 14, 2026

pllim reviewed Oct 14, 2026

View reviewed changes

pllim added the benchmark Run benchmarks for a PR label Oct 14, 2026

pllim reviewed Oct 15, 2026

View reviewed changes

Comment thread docs/changes/table/20599.perf.rst Resolved

ngoldbaum commented Oct 15, 2026

View reviewed changes

ngoldbaum mentioned this pull request Oct 18, 2026

DEP: deprecate implicit casts and promotion between text and bytes numpy/numpy#32757

Open

neutrinoceros reviewed Oct 20, 2026

View reviewed changes

taldcroft mentioned this pull request Oct 20, 2026

Table.from_df raises TypeError for pandas string columns with missing values (pd.NA) #20663

Open

ngoldbaum added 6 commits October 20, 2026 10:03

Avoid implicit bytes/text coercion in tables and metadata

6b86254

Respond to review and improve performance

5eddc77

Split performance release note into two notes

8841738

address clement's comments

4660219

Add docstrings and comments per Tom's request

f1f2ba6

more concise comments

145065c

ngoldbaum force-pushed the fix-implicit-text-bytes branch from f7815dc to 145065c Compare October 20, 2026 17:04

taldcroft approved these changes Oct 20, 2026

View reviewed changes

taldcroft merged commit b9cde4e into astropy:main Oct 20, 2026
43 of 44 checks passed

taldcroft mentioned this pull request Nov 8, 2026

Make from_df and from_pandas agree on missing values in string columns #20805

Draft

1 task done



		def _get_string_dtype(value):
		"""Identify flat bytes-only or Unicode sequences without promotion."""

		not isinstance(item, (int, float, complex, np.number))
		and not isinstance(item, np.bool_)

	string_types = (str, bytes)
	nan = np.nan
	if all(isinstance(x, string_types) or x is nan for x in data):

Conversation

ngoldbaum commented Oct 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

AI Disclosure

Merge method

Uh oh!

giles Bot commented Oct 14, 2026

Notes for maintainers

Uh oh!

github-actions Bot commented Oct 14, 2026

Uh oh!

ngoldbaum commented Oct 14, 2026

Uh oh!

mhvk commented Oct 14, 2026

Uh oh!

mhvk commented Oct 14, 2026

Uh oh!

taldcroft commented Oct 14, 2026

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ngoldbaum commented Oct 14, 2026

Uh oh!

pllim commented Oct 14, 2026

Uh oh!

pllim commented Oct 14, 2026

Uh oh!

ngoldbaum commented Oct 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim commented Oct 15, 2026

Uh oh!

neutrinoceros left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taldcroft commented Oct 20, 2026

Uh oh!

ngoldbaum commented Oct 20, 2026

Uh oh!

ngoldbaum commented Oct 14, 2026 •

edited

Loading

ngoldbaum commented Oct 14, 2026 •

edited

Loading

neutrinoceros commented Oct 20, 2026 •

edited

Loading