Skip to content

Derive ARCFileReader from G3Reader#188

Closed
arahlin wants to merge 10 commits intomasterfrom
arc_reader_subclass
Closed

Derive ARCFileReader from G3Reader#188
arahlin wants to merge 10 commits intomasterfrom
arc_reader_subclass

Conversation

@arahlin
Copy link
Copy Markdown
Member

@arahlin arahlin commented Mar 19, 2025

This ensures that the ARC file reader has the same arguments and functionality as the core G3Reader, and illustrates more generally how to create derived classes for creating streams of frames from other on-disk data formats.

The G3Reader class is refactored so that all of the logic surrounding opening file streams and counting frames is internal to the class, and a FillFrame function is exposed to derived classes to read the stream object and return a populated frame. The required file extension (modulo compression) is an optional keyword argument that is not exposed to python.
The stream object, current file name and frame counter variables are also visible to derived classes.

The ARCFileReader derived class then just configures the file extension in the base class constructor, and overrides the FillFrame function to parse arc data from the steam to populate GCPSlow frames, taking care to parse header information each time a new file is encountered in the stream.

This ensures that the ARC file reader has the same arguments and functionality
as the core G3Reader.

This also enables removing the dataio.h header from the public API.
@arahlin arahlin requested a review from nwhitehorn March 19, 2025 23:49
@arahlin arahlin self-assigned this Mar 19, 2025
Copy link
Copy Markdown
Member

@nwhitehorn nwhitehorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand why this is wanted. It seems like it mixes a lot of things together in order to make a header private.

@arahlin
Copy link
Copy Markdown
Member Author

arahlin commented Mar 25, 2025

I'm not sure I understand why this is wanted. It seems like it mixes a lot of things together in order to make a header private.

The G3Reader has a lot of options that aren't exposed for the ARCFileReader, and this is mainly fixing that. The header doesn't have to be private if you prefer it to remain part of the public API. I made the header private because the code there has been in flux and this would limit the number of other libraries that any code changes there would touch, but maybe that's a moot point with #189

@nwhitehorn
Copy link
Copy Markdown
Member

I don't really like the mixing of G3Reader (which reads .g3 files) with an entirely different type of file reader. It seems like it might make things more fragile in the end by trying to serve too many goals with G3Reader. I would personally prefer to revisit this after #189.

@arahlin
Copy link
Copy Markdown
Member Author

arahlin commented Mar 25, 2025

FWIW there is nothing in the G3Reader code that enforces file extensions or anything to indicate that it's specifically for G3 files. Perhaps that's something to add.

@arahlin arahlin force-pushed the arc_reader_subclass branch from 6d3b1f7 to adaabb3 Compare March 29, 2025 19:15
@arahlin arahlin force-pushed the arc_reader_subclass branch from adaabb3 to a1b3360 Compare March 29, 2025 20:37
@arahlin arahlin force-pushed the arc_reader_subclass branch from 75e52c3 to e96cc3c Compare March 31, 2025 14:39
@nwhitehorn
Copy link
Copy Markdown
Member

I'm still really not sure I understand the purpose of this. The point of having all the g3stream stuff is to abstract away the important details of G3Reader so that they can be shared with classes reading things that aren't .g3 files (like the ARC reader). Having the ARC reader inherit from a class that does something else entirely in order to use its wrappers around an abstraction seems architecturally kind of questionable to me. I feel like there has to be something I am missing here...

@arahlin arahlin closed this Apr 6, 2025
@arahlin arahlin deleted the arc_reader_subclass branch April 6, 2025 02:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants