Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge signed and unsigned integer types #1736

Merged
merged 1 commit into from
Jan 21, 2025
Merged

Merge signed and unsigned integer types #1736

merged 1 commit into from
Jan 21, 2025

Conversation

axelboc
Copy link
Contributor

@axelboc axelboc commented Jan 21, 2025

The goal is to split NumericType into two dtypes: IntegerType and FloatType, and to replace DTypeClass.Unsigned with a signed boolean in IntegerType.

I had first planned to introduce this change as part of adding support for big integers (cf. #1679 (comment)). At the time I wanted to introduce a new DType called BigIntegerType and it didn't really make sense to have this dtype together with NumericType... I'm now going back on this solution for reasons I'll explain soon, but I find that the NumericType refactoring still makes sense. IntegerType/FloatType brings us closer to the HDF5 types, and representing the sign as a boolean avoids relying on the DTypeClass enum's value too much in the metadata viewer (i.e. "Integer (unsigned)").

packages/app/src/metadata-viewer/utils.ts Show resolved Hide resolved
@@ -121,6 +120,7 @@ export type Endianness = (typeof H5T_TO_ENDIANNESS)[H5T_ORDER];
export type CharSet = (typeof H5T_TO_CHAR_SET)[H5T_CSET];
export type StrPad = (typeof H5T_TO_STR_PAD)[H5T_STR];

export type NumericType = IntegerType | FloatType;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We still keep a NumericType as that's still useful throughout the codebase.

@axelboc axelboc requested a review from loichuder January 21, 2025 13:54
@loichuder
Copy link
Member

I am not awfully convinced by this. Personally, I have always valued descriptive values over flags.

Also

IntegerType/FloatType brings us closer to the HDF5 types

Is that really true ? As far as I can see, unsigned and signed integers are separate datatypes: https://support.hdfgroup.org/documentation/hdf5/latest/group___p_d_t_s_t_d.html

So unless this lays ground for another future refactoring, I need a little more to go along 😄

@axelboc
Copy link
Contributor Author

axelboc commented Jan 21, 2025

@axelboc
Copy link
Contributor Author

axelboc commented Jan 21, 2025

So the idea is to bring DTypeClass closer to H5T_class_t. The only two values that don't have equivalent are DTypeClass.Bool and DTypeClass.Complex, which are compound types with very specific, well-defined fields.

Another argument is that the class property is the discriminant property of the discriminated union type DType. That one of the interfaces in that union has class: DTypeClass.Integer | DTypeClass.Unsigned | DTypeClass.Float is a bit of a conflicting pattern.

(I'd like to split ArrayType into two, ArrayType and VLenType, for the same reasons.)

Copy link
Member

@loichuder loichuder left a comment

Choose a reason for hiding this comment

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

Ok then 👍

@axelboc axelboc merged commit 77b3ebf into main Jan 21, 2025
8 checks passed
@axelboc axelboc deleted the merge-int-types branch January 21, 2025 15:56
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