BUG: reject inconsistent converters to StringDType by neutrinoceros · Pull Request #20762 · astropy/astropy

neutrinoceros · 2026-11-02T14:21:56Z

Description

Salvaged from gh-20744
The test already passes on main. However, I'm still including a companion refactor to avoid brittle substring matching
(the existing code would have broken when StringDType was introduced in numpy 2 if the class name had been stringdtype, for instance)

AI Disclosure

N/A

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.

github-actions · 2026-11-02T14:22:11Z

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.

taldcroft · 2026-11-03T09:18:18Z

    # Infer converter type from an instance of numpy_type.
    type_name = np.array([], dtype=numpy_type).dtype.name


This line is dead now and the comment also doesn't match the code now.

indeed. Fixed

taldcroft · 2026-11-03T09:20:30Z

+            converter_type = FloatType
+        case "b":
+            converter_type = BoolType
+        case "U":


Suggested change

case "U":

case "U" | "T":

If we are doing this then it probably makes sense to treat "T" as string-like. For instance:

lines = ["a\tb", "S\tN", "x\t1", "y\t2"] ascii.read(lines, format="rdb", converters={"b": [ascii.convert_numpy(str)]}) # StrType: rejected -> guess failure ascii.read(lines, format="rdb", converters={"b": [ascii.convert_numpy("T")]}) # AllType: accepted -> StringDType column

I'm not sure I understand your suggestion fully.
Here's what I'm grasping:

the str converter rejects the data

converting to StringDType however returns a result that to me looks valid

your suggestion would make both converters fail. I guess that's one way to resolve API inconsistencies, but I don't understand why that would be the behavior we expect. From where I'm standing, it actually looks like the str converter should be able to convert without raising here. Clearly we are not reaching the same conclusion, so am I missing anything ?

In this case the RDB format specifies that column b is numeric via the N (the "S\tN") line. That type requirement from the file is inconsistent with the user request to treat the column as string and gets rejected with an exception.

This behavior is consistent with the design philosophy / intent of io.ascii that the file format can specify the allowed type(s) and the user can only narrow those types, not go outside.

ah, I see. That makes a lot of sense, I just wasn't aware of the meaning caried by the second line.

neutrinoceros · 2026-11-03T13:34:32Z

@taldcroft with your suggestion taken into account, the scope changed a bit and this now is a bugfix. I've applied backport labels but if you think that's too disruptive for a backport I think it's also fine not to. Up to you.

taldcroft · 2026-11-03T15:04:52Z

@taldcroft with your suggestion taken into account, the scope changed a bit and this now is a bugfix. I've applied backport labels but if you think that's too disruptive for a backport I think it's also fine not to. Up to you.

I don't honestly think this would affect any code, but you can probably guess that this falls in the category of a behavior change that I'd rather not backport. I left the milestone at 8.1 and took off the backport labels. Thanks

neutrinoceros · 2026-11-03T17:38:52Z

CI jobs that failed on network errors were rerun and passed. Is there anything else to do before this can be merged ?

taldcroft

This is basically good but just a couple of non-code fixes.

taldcroft · 2026-11-05T13:56:16Z

-    Exact example from #9701: When using ascii.read with both the names and
-    converters arguments, the converters dictionary ignores the user-supplied
+    Adapted from #9701: When using ascii.read with both the names and
+    converters arguments, the converters dictionary ignores the user-supplied    converters arguments, the converters dictionary ignores the user-supplied


This line got garbled.

taldcroft · 2026-11-05T13:57:02Z

@@ -1005,17 +1005,18 @@ def convert_numpy(numpy_type):
        the required type.
    """
    # Infer converter type from an instance of numpy_type.


This comment is no longer correct, please update.

github-actions Bot added the io.ascii label Nov 2, 2026

neutrinoceros added no-changelog-entry-needed Refactoring labels Nov 2, 2026

neutrinoceros added this to the v8.1.0 milestone Nov 2, 2026

neutrinoceros mentioned this pull request Nov 2, 2026

ENH: redesign astropy.io.ascii.Column.type as an enum, improving maintainability and fine-level handling for all expected cases #20744

Closed

1 of 2 tasks

neutrinoceros marked this pull request as ready for review November 2, 2026 15:05

neutrinoceros requested review from dhomeier and taldcroft as code owners November 2, 2026 15:05

taldcroft requested changes Nov 3, 2026

View reviewed changes

neutrinoceros force-pushed the io.ascii/tst/check-StringDType branch from 832fcfc to bebc190 Compare November 3, 2026 11:31

neutrinoceros added 3 commits November 3, 2026 14:27

TST: add test cases for reading StringDType columns from an ascii table

a408041

RFC: rewrite case switching logic as a match/case tree

9172a76

BUG: reject inconsistent converters to StringDType

14b9cf3

neutrinoceros force-pushed the io.ascii/tst/check-StringDType branch from bebc190 to 14b9cf3 Compare November 3, 2026 13:31

neutrinoceros changed the title ~~TST: add test case for reading a StringDType column from an ascii table~~ BUG: reject inconsistent converters to StringDType Nov 3, 2026

neutrinoceros added Bug backport-v7.2.x on-merge: backport to v7.2.x backport-v8.0.x on-merge: backport to v8.0.x and removed no-changelog-entry-needed labels Nov 3, 2026

taldcroft removed backport-v7.2.x on-merge: backport to v7.2.x backport-v8.0.x on-merge: backport to v8.0.x labels Nov 3, 2026

taldcroft requested changes Nov 5, 2026

View reviewed changes

		# Infer converter type from an instance of numpy_type.
		type_name = np.array([], dtype=numpy_type).dtype.name

Conversation

neutrinoceros commented Nov 2, 2026

Description

AI Disclosure

Merge method

Uh oh!

github-actions Bot commented Nov 2, 2026

Uh oh!

taldcroft Nov 3, 2026

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Nov 3, 2026

Choose a reason for hiding this comment

Uh oh!

taldcroft Nov 3, 2026

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Nov 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taldcroft Nov 3, 2026

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Nov 3, 2026

Choose a reason for hiding this comment

Uh oh!

neutrinoceros commented Nov 3, 2026

Uh oh!

taldcroft commented Nov 3, 2026

Uh oh!

neutrinoceros commented Nov 3, 2026

Uh oh!

taldcroft left a comment

Choose a reason for hiding this comment

Uh oh!

taldcroft Nov 5, 2026

Choose a reason for hiding this comment

Uh oh!

taldcroft Nov 5, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

neutrinoceros Nov 3, 2026 •

edited

Loading