Fix table indices serialization test failures on ARMv7 and improve coverage by taldcroft · Pull Request #19644 · astropy/astropy

taldcroft · 2026-05-02T12:02:22Z

Description

This pull request is to address fix test failures from #18703 on platforms like ARMv7 where int64 is not the native integer type. It also expands test coverage to include creating indices with a variety of dtypes to stress the system a bit more.

Fixes #19632

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-05-02T12:02:33Z

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

cool ! while we're at it, we should also cover cases with mixed dtypes, otherwise LGTM. Thank you

neutrinoceros · 2026-05-02T14:06:59Z

 from .test_table import SetupData

 available_engines = [BST, SortedArray]
+NATIVE_INT_NAME = np.array(0).dtype.name


a slightly more straightforward form of this

Suggested change

NATIVE_INT_NAME = np.array(0).dtype.name

NATIVE_INT_NAME = np.int_.__name__

~~np.int is deprecated since numpy 1.20. There is a reason I did it the original way.~~

Sorry, I missed the trailing _.

The suggestion was np.int_, which is not deprecated, but I personally prefer the np.array(0) route as just letting the data do the work.

While checking for a sign of deprecation (and indeed, there is none), I also realized that I incorrectly recalled np.int_ to represent the native integer type when it's actually just always equivalent to np.int64. Which means I don't know why it even exists. Live and learn.

https://numpy.org/devdocs/reference/arrays.scalars.html#numpy.int_

neutrinoceros · 2026-05-02T14:10:56Z

+@pytest.mark.parametrize("dtype", [np.int16, np.float32, np.int64, np.float64])
+def test_indices_roundtrip_various_dtypes(dtype):
+    """Test that serialization round-trip works for various index dtypes."""
+    t = Table()
+    t["a"] = np.array([1, 3, 2], dtype=dtype)
+    t["b"] = np.array([5, 6, 7], dtype=dtype)


also making sure we roundtrip tables with mixed dtypes correctly

Suggested change

@pytest.mark.parametrize("dtype", [np.int16, np.float32, np.int64, np.float64])

def test_indices_roundtrip_various_dtypes(dtype):

"""Test that serialization round-trip works for various index dtypes."""

t = Table()

t["a"] = np.array([1, 3, 2], dtype=dtype)

t["b"] = np.array([5, 6, 7], dtype=dtype)

DTYPES = [np.int16, np.int32, np.int64, np.float32, np.float64]

@pytest.mark.parametrize("dt1", DTYPES)

@pytest.mark.parametrize("dt2", DTYPES)

def test_indices_roundtrip_various_dtypes(dt1, dt2):

"""Test that serialization round-trip works for various index dtypes."""

t = Table()

t["a"] = np.array([1, 3, 2], dtype=dt1)

t["b"] = np.array([5, 6, 7], dtype=dt2)

This is only a waste of CPU and time. The output indices that are being serialized are always native int and they do not depend on the data type. Even my original test is not strictly telling us much, but at least it is only 4 types, instead you're suggestion is for 25. Tests need to provide demonstrable value since they are not free.

# %ECSV 1.0 # --- # datatype: # - {name: a, datatype: int16} <-- This is tested thoroughly elsewhere # - {name: b, datatype: float32} # - {name: __index__, datatype: int64} <-- Doesn't depend on input datatype # - {name: __index__1, datatype: int64} # meta: !!omap # - __table_indices__: # indices: # - colnames: [a] # index_colname: __index__ # - colnames: [a, b] # index_colname: __index__1 # primary_key: [a] # schema: astropy-2.0 a b __index__ __index__1 1 5.0 0 0 3 6.0 2 2 2 7.0 1 1

I agree that we should not go overboard with testing, if only to save some energy costs (aside: this is partially why I like so much the idea of moving the compiled code out of astropy).

neutrinoceros · 2026-05-02T14:16:12Z

-      2 3 2
-      3 5 0
-      1 4 1
+    See the ``test_indices_serialization_*` tests in ``test_index.py`` for examples.


I'm not sure there's a precedent for referencing tests directly in a docstring. This doesn't seem right to me: it would bitrot silently if we were to remove tests from wheels, for instance (not likely to happen soon, but not an impossibility on the long term). I suggest using the patch from #19634 here instead.

For all intents and purposes this is an internal function that only developers will be checking. You are going to say it should be explicit with _ but that's just not the way this code has been developed over 15 years. This level of fear about inconsequential changes that have both low impact and low probability is not warranted.

The example here was helpful, but I can see the case for feeling that having to make it work for different architectures is beyond what is reasonable for a helper function. I do like having a little hint where to look here, and our tests have historically been pretty much invariant, so this seems fine to me.

mhvk

@taldcroft - arrgg, 32 bit integers again come back to haunt us! Your solution here looks good to me.

mhvk · 2026-05-02T19:43:11Z

 from .test_table import SetupData

 available_engines = [BST, SortedArray]
+NATIVE_INT_NAME = np.array(0).dtype.name


The suggestion was np.int_, which is not deprecated, but I personally prefer the np.array(0) route as just letting the data do the work.

mhvk · 2026-05-02T19:47:13Z

+@pytest.mark.parametrize("dtype", [np.int16, np.float32, np.int64, np.float64])
+def test_indices_roundtrip_various_dtypes(dtype):
+    """Test that serialization round-trip works for various index dtypes."""
+    t = Table()
+    t["a"] = np.array([1, 3, 2], dtype=dtype)
+    t["b"] = np.array([5, 6, 7], dtype=dtype)


I agree that we should not go overboard with testing, if only to save some energy costs (aside: this is partially why I like so much the idea of moving the compiled code out of astropy).

mhvk · 2026-05-02T19:49:43Z

-      2 3 2
-      3 5 0
-      1 4 1
+    See the ``test_indices_serialization_*` tests in ``test_index.py`` for examples.


The example here was helpful, but I can see the case for feeling that having to make it work for different architectures is beyond what is reasonable for a helper function. I do like having a little hint where to look here, and our tests have historically been pretty much invariant, so this seems fine to me.

neutrinoceros · 2026-05-02T20:10:41Z

It doesn't look like any of my suggestions, comments and questions have produced anything other than frustration for all parties involved. If small improvements and a critical perspective on 15 years old practices are both deemed undesirable, then I do not know what to offer as a reviewer.
I don't feel good writing this. Please dismiss my review since it's not bringing any value.

@neutrinoceros

@neutrinoceros requested his review be dismissed

Fix test failures on ARMV7 and improve coverage

2d5dd34

taldcroft requested a review from neutrinoceros as a code owner May 2, 2026 12:02

github-actions Bot added table unified-io labels May 2, 2026

taldcroft mentioned this pull request May 2, 2026

armv7 CI failures with recent serialization changes #19632

Closed

taldcroft added Affects-dev PRs and issues that do not impact an existing Astropy release no-changelog-entry-needed labels May 2, 2026

taldcroft added this to the v8.0.0 milestone May 2, 2026

astrofrog added the Extra CI Run cron CI as part of PR label May 2, 2026

neutrinoceros previously requested changes May 2, 2026

View reviewed changes

neutrinoceros reviewed May 2, 2026

View reviewed changes

neutrinoceros mentioned this pull request May 2, 2026

TST: fix portability issue with new serialization tests #19634

Closed

1 task

neutrinoceros added the testing label May 2, 2026

taldcroft requested a review from mhvk May 2, 2026 17:53

mhvk approved these changes May 2, 2026

View reviewed changes

taldcroft enabled auto-merge (squash) May 2, 2026 20:45

taldcroft merged commit 2189db6 into astropy:main May 2, 2026
74 of 80 checks passed

	NATIVE_INT_NAME = np.array(0).dtype.name
	NATIVE_INT_NAME = np.int_.__name__

Conversation

taldcroft commented May 2, 2026

Description

Uh oh!

github-actions Bot commented May 2, 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!

taldcroft May 2, 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!

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!

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

neutrinoceros commented May 2, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

taldcroft May 2, 2026 •

edited

Loading