Fix #8490: FITS serialization for SunPy coordinates by Amityush-lgtm · Pull Request #8548 · sunpy/sunpy

Amityush-lgtm · 2026-03-21T21:08:48Z

PR Description

This PR resolves an issue where writing or reading SunPy coordinate frames to and from FITS files using Astropy tables (QTable) was crashing due to missing serialization registrations.

What Changed:

Added init_subclass to SunPyBaseCoordinateFrame in sunpy/coordinates/frames.py. This ensures that any new coordinate frame subclass is automatically registered for serialization without needing manual configuration.
The init_subclass now configures Astropy’s QTable processing and registers the correct PyYAML handlers so frames save and load exactly as they should.
Added test_table_serialization.py to properly verify FITS reading and writing for both single observer and array observer.

AI Assistance Disclosure

AI tools were used for:

Code generation (e.g., when writing an implementation or fixing a bug)
Test/benchmark generation
Documentation (including examples)
Research and understanding
No AI tools were used

AI was used to help draft the initial version of the coordinate serialization fix and some unit tests. I have fully reviewed and manually verified the behaviour via tests

Regardless of AI use, the human contributor remains fully responsible for correctness, design choices, licensing compatibility, and long-term maintainability.

@samaloney, Let me know your thoughts on this. Happy to make changes!

ayshih · 2026-03-21T21:22:04Z

If such registration is necessary, can we do it within __init_subclass__() for SunPyBaseCoordinateFrame? Or better yet, push it upstream to BaseCoordinateFrame?

Amityush-lgtm · 2026-03-21T21:33:12Z

@ayshih Thank you for your guidance, Using init_subclass on SunPyBaseCoordinateFrame would be much better than my current approach it would automatically register any new frames without needing a separate module.

Amityush-lgtm · 2026-03-21T21:35:52Z

for pushing it upstream to BaseCoordinateFrame in Astropy, will that be in the scope of this PR??

ayshih · 2026-03-22T01:23:22Z

It'd be fine to fix it here, and then try to push it upstream to Astropy at some future date. How does Astropy register their built-in frames, and if it's not through __init_subclass__() on BaseCoordinateFrame, why not? Let's check for any gotchas before we proceed with __init_subclass__().

Amityush-lgtm · 2026-03-22T07:03:02Z

@ayshih I looked into how Astropy handles this. It doesn't use __init_subclass__() on BaseCoordinateFrame. Instead in _construct_mixin_from_obj_attrs_and_info method serialize.py (L354-358) uses a prefix check.

Amityush-lgtm · 2026-03-22T07:25:15Z

Astropy's own frames like ICRS, Galactic pass through because they all start with astropy.coordinates.builtin_frames, while SunPy frames fail because sunpy.coordinates.frames.* doesn't match that prefix.

samaloney

Nice

samaloney · 2026-03-23T09:20:28Z

+            if isinstance(cls, type) and issubclass(cls, BaseCoordinateFrame):
+                key = f"{cls.__module__}.{cls.__name__}"
+                frame_classes[key] = cls
+    return tuple(frame_classes.values())


Why create the a dictionary if just returning the values tuple? I think if you did return the dictionary here you could avoid having to recreate it later in ~L43 e.g f"{cls.__module__}.{cls.__name__}"?

samaloney · 2026-03-23T09:22:30Z

+        from astropy.io.misc.yaml import AstropyDumper, AstropyLoader
+    except ImportError:
+        # PyYAML is optional in Astropy, so skip registration if unavailable.
+        return


I'm pretty sure it's core dependency of astropy?

samaloney · 2026-03-23T09:30:43Z

Yea the relevant code block as linked to by @Amityush-lgtm

    # We translate locally created skyoffset frames and treat all
    # built-in frames as known.
    if cls_full_name.startswith("abc.SkyOffset"):
        cls_full_name = "astropy.coordinates.SkyOffsetFrame"
    elif (
        cls_full_name not in __construct_mixin_classes
        and not cls_full_name.startswith("astropy.coordinates.builtin_frames")
    ):
        raise ValueError(f"unsupported class for construct {cls_full_name}")

and since we can't change the namespace of our coords it looks like the only way is to add it to __construct_mixin_classes which is created from a hard coded list at the top of the file https://github.com/astropy/astropy/blob/905b4262359f7b0384f36170be96de6e2558ceab/astropy/table/serialize.py#L14-L79

Amityush-lgtm · 2026-03-23T09:51:14Z

Thanks for the review @samaloney I have implemented the approach as per @ayshih suggestion i replaced _table_yaml.py with init_subclass() on SunPyBaseCoordinateFrame so any new frame subclass is automatically registered also updated the test to use np.arange instead of zeros. Will be pushing soon!

Amityush-lgtm · 2026-03-23T10:06:12Z

Done! Let me know your thoughts on this @ayshih @samaloney
Happy to make changes

samaloney

I think this nearly good to go, just few small comments

Amityush-lgtm · 2026-03-26T21:10:42Z

@samaloney, I have made the suggested changes.

ayshih · 2026-04-01T18:53:24Z

+        if hasattr(table_serialize, "__construct_mixin_classes"):
+            existing_classes = getattr(table_serialize, "__construct_mixin_classes")
+            if cls_name not in existing_classes:
+                setattr(table_serialize, "__construct_mixin_classes", existing_classes + (cls_name,))


I think it's safe to assume that __construct_mixin_classes exists. I also think that using getattr()/setattr() is only useful if we are anticipating __construct_mixin_classes not being a variable, but it's difficult to guess the future. Can we keep things simple:

Suggested change

if hasattr(table_serialize, "__construct_mixin_classes"):

existing_classes = getattr(table_serialize, "__construct_mixin_classes")

if cls_name not in existing_classes:

setattr(table_serialize, "__construct_mixin_classes", existing_classes + (cls_name,))

if cls_name not in table_serialize.__construct_mixin_classes:

table_serialize.__construct_mixin_classes += (cls_name,)

On writing table_serialize.__construct_mixin_classes directly inside throws an AttributeError.

I guess using getattr() might solve this.

Let me know your thoughts @ayshih

Argh, the problem is that the variable name has two leading underscores, which unfortunately triggers Python's private-name-mangling logic when accessed in a class definition. I suggest adding a comment for future maintenance.

You can still remove the hasattr() check.

Suggested change

if hasattr(table_serialize, "__construct_mixin_classes"):

existing_classes = getattr(table_serialize, "__construct_mixin_classes")

if cls_name not in existing_classes:

setattr(table_serialize, "__construct_mixin_classes", existing_classes + (cls_name,))

# We have to use getattr()/setattr() to avoid triggering Python name mangling

existing_classes = getattr(table_serialize, "__construct_mixin_classes")

if cls_name not in existing_classes:

setattr(table_serialize, "__construct_mixin_classes", existing_classes + (cls_name,))

Got it! I've updated the PR to use getattr() and added the comment.

samaloney

I think this is good to go

Amityush-lgtm · 2026-04-16T10:35:16Z

Thanks for taking the time to review this

Co-authored-by: Shane Maloney <[email protected]>

Cadair · 2026-04-22T15:26:53Z

I'm going to have to vote -0 on this, the setting of a variable which someone decided to prefix with a double underscore feels really dirty.

I feel like this is something we should really work with astropy to fix properly, i.e. address astropy/astropy#13689.

Co-authored-by: Albert Y. Shih <[email protected]>

ayshih

This approach is not ideal, but I feel that it is acceptable

Amityush-lgtm · 2026-04-22T19:56:16Z

Thank you for the review and the guidance @ayshih . I agree its not a long term fix but i'm glad its acceptable for now and I'll keep an eye on that Astropy issue for a cleaner fix in the future.

samaloney · 2026-04-22T21:27:31Z

Hum the py312-oldestdeps fail seems real and related to this PR, going to assume something different in older version of astropy==6.1.0?

Amityush-lgtm · 2026-04-23T09:30:28Z

@samaloney I looked into the failure and found that older Astropy versions don't have the .info part that is used here.

Im thinking to update the code to handle this like manually collect all the frame's data like longitude, latitude and observation time and put them into a dictionary so it works on older versions and fix the tests to skip a few tricky cases that only work on newer Astropy.

Will this be fine? Let me know your thoughts

samaloney · 2026-04-23T10:09:21Z

Hey @Amityush-lgtm ok that make sense, what version of astropy was .info added in? @Cadair when is our next planned bump of the astropy dependency?

Cadair · 2026-04-23T10:11:26Z

8.0 should have a minimum of 6.1

Amityush-lgtm · 2026-04-23T18:09:32Z

Another failure in oldestdeps was due to older Astropy using .get_name() instead of .name for the representation type class. Also the ascii.ecsv format was failing to serialize array mixins on older versions. I've tried to update the logic.

Amityush-lgtm · 2026-04-30T21:47:19Z

@samaloney The oldestdeps fix worked. The remaining failures are unrelated to coordinate serialization, let me know if any other changes are required.

Amityush-lgtm added a commit to Amityush-lgtm/sunpy that referenced this pull request Mar 21, 2026

Rename changelog to match PR sunpy#8548

f9afa7e

samaloney reviewed Mar 23, 2026

View reviewed changes

Amityush-lgtm force-pushed the fix-coordinate-serialization branch from 85ba7ea to ecde9cf Compare March 23, 2026 09:59

samaloney reviewed Mar 23, 2026

View reviewed changes

Comment thread sunpy/coordinates/frames.py Resolved

Amityush-lgtm mentioned this pull request Mar 25, 2026

SunPy: Improving radiospectra's functionality and interoperability (Kumar Amityush) OpenAstronomy/gsoc-proposals#3

Merged

5 tasks done

samaloney requested changes Mar 26, 2026

View reviewed changes

Comment thread sunpy/coordinates/tests/test_table_serialization.py Resolved

Comment thread sunpy/coordinates/tests/test_table_serialization.py Resolved

Comment thread sunpy/coordinates/frames.py Resolved

Amityush-lgtm requested a review from samaloney March 26, 2026 21:10

samaloney reviewed Apr 1, 2026

View reviewed changes

Comment thread sunpy/coordinates/tests/test_table_serialization.py Resolved

samaloney reviewed Apr 1, 2026

View reviewed changes

Comment thread changelog/8548.bugfix.rst Resolved

Amityush-lgtm requested a review from samaloney April 1, 2026 17:02

ayshih reviewed Apr 1, 2026

View reviewed changes

samaloney approved these changes Apr 16, 2026

View reviewed changes

Amityush-lgtm and others added 7 commits April 18, 2026 15:53

Fix FITS serialization for SunPy coordinates (sunpy#8490)

31ef9b3

Add changelog for sunpy#8490

ec9d7d7

Rename changelog to match PR sunpy#8548

2c0dfc0

Implement __init_subclass__ for frame serialization registration

530a26b

Suggested changes

026ad0e

Co-authored-by: Shane Maloney <[email protected]>

Applying suggested changes

6e926a5

updating changelog

e622012

Amityush-lgtm added 2 commits April 18, 2026 15:53

Add ECSV and Parquet to table serialization tests

afffb69

using getattr/setattr to avoid Python name mangling

5cc57d4

samaloney force-pushed the fix-coordinate-serialization branch from 7d1ca71 to 5cc57d4 Compare April 18, 2026 14:53

Merge branch 'main' into fix-coordinate-serialization

2c75a18

Cadair added this to the 8.0.0 milestone Apr 22, 2026

Cadair added coordinates Affects the coordinates submodule No Backport A PR that isn't to be backported to any release branch. (To be used as a flag to other maintainers) labels Apr 22, 2026

ayshih reviewed Apr 22, 2026

View reviewed changes

Comment thread sunpy/coordinates/frames.py Resolved

Applying suggestions

63c0468

Co-authored-by: Albert Y. Shih <[email protected]>

Cadair reviewed Apr 22, 2026

View reviewed changes

Comment thread changelog/8548.bugfix.rst Resolved

Rename changelog from bugfix to feature

fe174e3

ayshih approved these changes Apr 22, 2026

View reviewed changes

samaloney mentioned this pull request Apr 22, 2026

change __construct_mixin_classes to some type of registration astropy/astropy#13689

Open

Merge branch 'main' into fix-coordinate-serialization

74015a8

Amityush-lgtm added 2 commits April 23, 2026 18:26

Fix coordinate serialization fallback for Astropy < 7.0

dbccd01

Fix oldestdeps representation fallbacks

5c57324

Merge branch 'main' into fix-coordinate-serialization

4355989

alasdairwilson approved these changes May 7, 2026

View reviewed changes

Merge branch 'main' into fix-coordinate-serialization

e6a5e42

ayshih enabled auto-merge (squash) May 8, 2026 19:00

ayshih merged commit 74cdad8 into sunpy:main May 8, 2026
49 of 57 checks passed

Cadair added the Whats New? Needs a section added to the current Whats New? page. label May 27, 2026

Conversation

Amityush-lgtm commented Mar 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Description

AI Assistance Disclosure

Uh oh!

ayshih commented Mar 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Amityush-lgtm commented Mar 21, 2026

Uh oh!

Amityush-lgtm commented Mar 21, 2026

Uh oh!

ayshih commented Mar 22, 2026

Uh oh!

Amityush-lgtm commented Mar 22, 2026

Uh oh!

Amityush-lgtm commented Mar 22, 2026

Uh oh!

samaloney left a comment

Choose a reason for hiding this comment

Uh oh!

samaloney Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

samaloney Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

samaloney commented Mar 23, 2026

Uh oh!

Amityush-lgtm commented Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Amityush-lgtm commented Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

samaloney left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Amityush-lgtm commented Mar 26, 2026

Uh oh!

Uh oh!

Uh oh!

ayshih Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Amityush-lgtm Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Amityush-lgtm Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

ayshih Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Amityush-lgtm Apr 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!

samaloney left a comment

Choose a reason for hiding this comment

Uh oh!

Amityush-lgtm commented Apr 16, 2026

Uh oh!

Cadair commented Apr 22, 2026

Uh oh!

Uh oh!

Uh oh!

ayshih left a comment

Choose a reason for hiding this comment

Uh oh!

Amityush-lgtm commented Apr 22, 2026

Uh oh!

samaloney commented Apr 22, 2026

Uh oh!

Amityush-lgtm commented Mar 21, 2026 •

edited

Loading

ayshih commented Mar 21, 2026 •

edited

Loading

Amityush-lgtm commented Mar 23, 2026 •

edited

Loading

Amityush-lgtm commented Mar 23, 2026 •

edited

Loading

Amityush-lgtm Apr 2, 2026 •

edited

Loading

Amityush-lgtm commented Apr 30, 2026 •

edited

Loading