ENH: redesign `astropy.io.ascii.Column.type` as an enum, improving maintainability and fine-level handling for all expected cases by neutrinoceros · Pull Request #20744 · astropy/astropy

neutrinoceros · 2026-10-30T14:47:56Z

Description

Part of #18388
This started as an excuse for a deep dive in the conversion machinery. Now I think it genuinely improves maintainability with two main benefits:

centralizing information: the previous model relied on a class hierarchy, which is now represented by a single and simple enumeration
allowing for exhaustive checks (i.e., typing.assert_never)

Another benefit of this side quest is that I learned a lot about the core of io.ascii.

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-10-30T14:48: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.

neutrinoceros · 2026-10-30T15:07:17Z

                )
            except Exception as exc:
-                raise ValueError(f"column {col.name!r} failed to convert: {exc}")
+                raise ValueError(


not strictly related, just a fly-by improvement. Happy to split it out on request

neutrinoceros · 2026-10-30T15:08:11Z

+                        # bug-for-bug backward compat
+                        fillval = "0"


temporary comment.
Question to reviewers: shouldn't this be

Suggested change

# bug-for-bug backward compat

fillval = "0"

fillval = "0.0"

?

Originally, we used "0" because the same value can be used as a string, int, and float. That's convenient for setting the fill_value because you can universally set the same value for all columns, even for bool columns (since "0" evaluates to a bool value in "if 0").

That's what the comment "bug-for-bug backward compat" means. It might not be the most elegant choice, but it's been a choice for 15 years, so we would need very, very good reasons to change it. "Looks odd" isn't good enough. Would we write this like this today? No. But is that good enough a reason to break backwards compatibility by changing it? Absolutely not.

And (in this and similar comments), it's important to remember that in the most modern use the fill value doesn't matter at all, because you should use a masked table if values are indeed masked by using a fill value.

That's what the comment "bug-for-bug backward compat" means.

indeed. I wrote this first to raise awareness and ensure it was discussed in review, but it's a perfectly valid option to just keep it there too, if that's your preference.

neutrinoceros · 2026-10-30T15:10:15Z

+                        # bug-for-bug backward compat
+                        fillval = "0"


also strictly equivalent to the existing code, but suspicious (to me). Do we really want to represent a boolean False as '0', and how is that consistent with BOOL not being considered a subtype to NUMERIC ?

neutrinoceros · 2026-10-30T15:11:03Z

+                    case core.ColumnType.ANY | core.ColumnType.UNDEFINED:
+                        raise ValueError


this branch is completely new. I believe it is correct to raise in both cases, which wasn't considered before

neutrinoceros · 2026-10-30T15:12:25Z

+                    case _ as unreachable:
+                        assert_never(unreachable)


thanks to the addition of the previous branch, we can now check at type-check time that all possible cases are covered. In other words, we can check that the match/case tree is exhaustive

neutrinoceros · 2026-10-30T15:13:23Z

                raise TypeError(
                    f"could not convert reader output to {cls.__name__} class."
-                )
+                ) from exc


not strictly related, just a fly-by improvement. Happy to split it out on request

neutrinoceros · 2026-10-30T15:16:22Z

-    type does not match ``StrType``, ``FloatType``, or ``IntType``.
-    """
+class ColumnType(Enum):
+    ANY = auto()  # top type


this is the equivalent of NoType in the previous version, but using a conventional name for a top type

neutrinoceros · 2026-10-30T15:19:25Z

+    FLOAT = auto()
+    INTEGER = auto()
+    BOOL = auto()
+    UNDEFINED = auto()  # bottom type


this is the equivalent of AllType in the previous version, with the benefit that it cannot be made inconsistent with the rest of the model, where AllType could break if a type was added to the hierarchy but not made a parent to it.

hamogu

Everything in this PR makes sense and everything makes the code better. Still, I think the entire PR should be rejected (with the possible exception of the "from None" and "from exec" in the lines where an error is thrown).

As I detail in one of my answers to specific comments, this module is old and both Python and we as developers have learned things since it was written. There were no "switch" statements, no type hierarchy that put "numeric" on the top and no "from None" for exceptions.

However, reading ascii tables is also one of the top subpackages in astropy by use as far as we know. There are literally thousands of astronomers out there that use it to read their tables, from undergrad students to operation scripts of major space telescopes and somewhere someone relies on the fact wording of an error or the exact (unconventional, user defined) "FloatType". So we need to be very, very careful anytime we change something, even if it seems trivial like changing an error message from "convert FloatType" to "convert Float".
Of course, we want to fix bugs that stop people from using this (such as crashes, not-helpful errors, or MRT tables that the AAS would accept for publications because it's not the format they need), but we need to avoid changes for the sake of change at all costs.

It's not clear what the use case for this PR is beyond "it's better now", which it undeniably is, but how does a user profit enough to warrant breaking things? How does changing a mask value from "0" to "0.0" help a user? Purists might shutter, but "0" in float columns has worked for more than a decade and I don't see a reason why masking float columns with "0" should break in the foreseeable future.

While the old code is not the easiest to read, the improvements are marginal in the new code (because it has to adhere closely to the existing code).

I don't want to close this PR out of hand because the PR comes after careful work from an experienced developer, but without a clear need and obvious user-facing benefits, I'm opposed to changing things.

hamogu · 2026-10-31T01:06:29Z

        assert len(w) == 4
        for i in range(len(w)):
-            assert f"OverflowError converting to FloatType in column col{i + 2}" in str(
+            assert f"OverflowError converting to Float in column col{i + 2}" in str(


I'm not a big fan of changes, even if it's only an error message, that don't have a clear purpose. The old message was understandable to users, and so is the new one. I get that "FloatType" is not how we typically write things in Python today, but is that really worth the change? This is a 15 year old module and somewhere someone is checking the error message to catch something in their code and this breaks it needlessly.

In other cases, there is a clear improvement that justifies a change (e.g. adding text to tell users how to fix something or in which row or column the error occurs, but here is only saves 4 characters.

Personally, I would revert all changes this and similar error messages that were changes in this PR.

hamogu · 2026-10-31T01:18:33Z

+                        # bug-for-bug backward compat
+                        fillval = "0"


Originally, we used "0" because the same value can be used as a string, int, and float. That's convenient for setting the fill_value because you can universally set the same value for all columns, even for bool columns (since "0" evaluates to a bool value in "if 0").

That's what the comment "bug-for-bug backward compat" means. It might not be the most elegant choice, but it's been a choice for 15 years, so we would need very, very good reasons to change it. "Looks odd" isn't good enough. Would we write this like this today? No. But is that good enough a reason to break backwards compatibility by changing it? Absolutely not.

And (in this and similar comments), it's important to remember that in the most modern use the fill value doesn't matter at all, because you should use a masked table if values are indeed masked by using a fill value.

neutrinoceros · 2026-10-31T07:23:52Z

thanks for your prompt, detailed and honest answer. I'm switching this to draft while I iron out the bits you like least. In particular, I'm reverting changes to error messages and extracting the consensual parts to dedicated PRs.

On the question of user impact, I believe that while indirect, it is the end goal here.
Using modern coding practices unlock new avenues for bug detection, which benefit both developers by making debugging easier, and users by hardening the code base, reducing the chances of a defect.

I should also share my entire plan so far: what I'm actually trying to achieve is to make the Column class slotted. Currently, this class has a lot of attributes that are never defined in the __init__ method, and instead are set at arbitrary points through constructors that don't live within the class. This makes it very hard to reason about the class and where it's used. Adding __slots__ not only create the missing single source of truth, it would also provide the following (dev side) benefits

improve code navigation (via LSPs)
make errors like misspelled attributes obvious at runtime

This PR started when I tried to add a type annotation to Column.type, which will be necessary to the purpose of making Column slotted. I could write a brittle and hardly readable annotation that describes the existing code, but I figured I might as well try not to, because the underneath model is actually very simple, just not written in a way modern tooling can leverage.

neutrinoceros · 2026-10-31T09:07:20Z

-    type does not match ``StrType``, ``FloatType``, or ``IntType``.
-    """
+    @property
+    def nickname(self) -> str:


@hamogu this property is the backward compatible layer I added in response to your concerns regarding error messages

taldcroft · 2026-10-31T09:12:53Z

I'd like to strongly second what @hamogu said. Basically this is legacy code and despite flaws in the original design it serves the community well. The maintenance burden with the current code has NOT been an issue and I foresee no problems going forward. Thus improving maintainability is not a driver for large-scale code changes.

In short, any proposals for making design changes to io.ascii should be put forth as an enhancement issue prior to starting work. Reviewing changes can be as much work as making them, so there needs to be a real driver from the user perspective to justify the changes. Supporting better type annotations is not a sufficient driver.

…intainability and fine-level handling for all expected cases2026-10-30 1

neutrinoceros · 2026-10-31T09:35:47Z

+            converter_type = ColumnType.FLOAT
+        case "b":
+            converter_type = ColumnType.BOOL
+        case "U":


@hamogu this is the faithful translation of the existing code but it begs the question: should np.dtype('T') (whose name is 'StringDType128') also match this branch ? This case didn't exist last time this code was edited, so it wasn't a defect then, but arguably it is one now. In any case I figure this defect is very minor and probably inconsequential since this function isn't supposed to be public, but it seems best to address the defect right now than to go through a painful rediscovery process if this ever has any undesirable effect, because it wouldn't be immediately visible

this comment remains to be discussed even with this PR rejected
@hamogu @taldcroft

nevermind, it's actually more a problem of test coverage, so I don't expect controversy on this point.
See gh-20762

neutrinoceros · 2026-10-31T20:59:50Z

Just to reiterate in case my point didn't make it through: this has nothing to do with modernizing for the sake of it and everything with making the code easier to reason about in order to fix real bugs down the road. The bugs I'm talking about do not have to live in the exact area I'm refactoring here. In fact I hope the whole exercise reduced that chance to ~0. The idea is that if this part is obviously correct, then the mental overhead it takes for someone reasoning about a bigger picture is virtually zero. I'm seeing it as an investment, and that's precisely why I take on this kind of tasks early in the process, before delivering more user-impacting patches, so it has a greater chance to pay off rapidly.

taldcroft · 2026-11-01T11:38:59Z

@neutrinoceros - thanks for thinking about io.ascii. However, your plans for big-picture improvements to the package do not align with my priorities as the original author of this package and current maintainer. To reiterate, if you'd like to discuss a plan for a design changes to io.ascii please open an issue or an APE.

In particular, package stability is the highest priority for me. The change to using Enum that is proposed here and your idea about moving to a slotted Column both will break custom 3rd party readers. The founding principle of io.ascii was extensibility to allow users to write their own readers that do not necessarily get brought into the core astropy. Since this has been advertised as a feature since io.ascii existed, we should assume that such extensions exist in the wild.

I also disagree with the need for this highly-impacting change for the sake of long-term maintenance. We have a lot of experience now and the package is stable. The current maintainers are managing without too much trouble and have not considered this a package that needs help. With appropriate use of AI, fixing bugs now is often quite easy and I already have fixes available for many of the existing bugs that I need to review internally.

My expectation for the future of io.ascii is to fix bugs, implement performance enhancements, and add new readers as warranted.

Here is the long form discussion about the API break from Claude which I agree with:

Public API removed with no deprecation. AllType, FloatType, IntType,
NoType, NumType, StrType disappear from astropy.io.ascii. They are more than
internal names. They are the vocabulary of the extension API:

the converter_type returned by the public convert_numpy, which callers compare
with issubclass;
the (func, type) tuple form of converters=. A user's
converters={"a": [(f, ascii.FloatType)]} now raises AttributeError;
the col_type_map of user-written BaseHeader subclasses (core.FloatType etc.).

This cannot be shimmed cleanly. Enum members can't stand in for classes under
issubclass, so a deprecation path would mean keeping both models for a release
cycle.

hamogu · 2026-11-01T12:02:04Z

I think @taldcroft and I agree that this PR is unsuitable for io.ascii and the discussion so far has not convinced me otherwise.

Thanks @neutrinoceros for throughly investigating other possibilities and comparing what could be done and what the potential advantages are. It's good to know what we're missing out on and this would a good starting point if we ever decided to do a "ascii 2.0", similar t the units team planning a "units 2.0". But for the current io.ascii package, this is too much change with too little need, thus I'm closing the PR.

neutrinoceros · 2026-11-01T13:32:08Z

this highly-impacting change

I should clarify here: I genuienely had no idea this would be considered impacting. My whole argument was based on the assumption that this code was purely internal. I would not knowingly propose breaking changes for the sake of maintainability.

neutrinoceros · 2026-11-01T13:36:13Z

parts of this PR were just adding type annotations to existing code without changing any API. The process wasn't always obvious and the result is not disrupting anything. Is it okay that I re-open another PR with just these ?

neutrinoceros · 2026-11-02T07:26:33Z

I would also like some feedback on #20744 (comment) which I believes highlights an actual defect of the code that should be addressed even with this particular PR rejected

neutrinoceros added this to the v8.1.0 milestone Oct 30, 2026

neutrinoceros added Enhancement Refactoring no-changelog-entry-needed typing related to type annotations labels Oct 30, 2026

github-actions Bot added table io.ascii unified-io labels Oct 30, 2026

neutrinoceros force-pushed the io.ascii/rfc/coltype-enum branch 2 times, most recently from 11c334a to eb45fdd Compare October 30, 2026 15:03

neutrinoceros commented Oct 30, 2026

View reviewed changes

neutrinoceros marked this pull request as ready for review October 30, 2026 15:20

neutrinoceros requested review from dhomeier and taldcroft as code owners October 30, 2026 15:20

neutrinoceros assigned hamogu Oct 30, 2026

neutrinoceros force-pushed the io.ascii/rfc/coltype-enum branch 6 times, most recently from 71db273 to abba522 Compare October 30, 2026 21:03

hamogu requested changes Oct 31, 2026

View reviewed changes

This was referenced Oct 31, 2026

UX: improve error chaining in io.ascii #20745

Merged

TST/RFC: improve test ids generated by a test fixture #20746

Merged

neutrinoceros marked this pull request as draft October 31, 2026 07:07

neutrinoceros force-pushed the io.ascii/rfc/coltype-enum branch from abba522 to 098d38b Compare October 31, 2026 08:56

neutrinoceros commented Oct 31, 2026

View reviewed changes

ENH: redesign astropy.io.ascii.Column.type as an enum, improving ma…

f39fdf0

…intainability and fine-level handling for all expected cases2026-10-30 1

neutrinoceros force-pushed the io.ascii/rfc/coltype-enum branch from 098d38b to f39fdf0 Compare October 31, 2026 09:27

neutrinoceros commented Oct 31, 2026

View reviewed changes

neutrinoceros marked this pull request as ready for review October 31, 2026 11:02

hamogu closed this Nov 1, 2026

hamogu mentioned this pull request Nov 2, 2026

Add support of float128 to io.ascii readers #20761

Open

neutrinoceros mentioned this pull request Nov 2, 2026

BUG: reject inconsistent converters to StringDType #20762

Open

1 of 2 tasks

		case core.ColumnType.ANY \| core.ColumnType.UNDEFINED:
		raise ValueError

Conversation

neutrinoceros commented Oct 30, 2026

Description

AI Disclosure

Merge method

Uh oh!

github-actions Bot commented Oct 30, 2026

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!

hamogu 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!

neutrinoceros commented Oct 31, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taldcroft commented Oct 31, 2026

Uh oh!

neutrinoceros Oct 31, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neutrinoceros commented Oct 31, 2026

Uh oh!

taldcroft commented Nov 1, 2026

Uh oh!

hamogu commented Nov 1, 2026

Uh oh!

neutrinoceros commented Nov 1, 2026

Uh oh!

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

Uh oh!

Uh oh!

neutrinoceros commented Nov 2, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

neutrinoceros Oct 31, 2026 •

edited

Loading

neutrinoceros commented Nov 1, 2026 •

edited

Loading