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

Fix calling get_attribute_names on committed datatype #84

Merged
merged 7 commits into from
Sep 5, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
# Changelog
## v0.7.8 TBC
### Fixed
- Fix accessing attributes of committed datatype with `my_datafile.attrs`.
- Fix calling `get_attribute_names` method of Module API on committed datatype.
### Changed
- Mark optional parameters as such in the TypeScript declarations of the following `H5Module` methods: `open`, `create_dataset`, `create_group`, `create_vlen_str_dataset` and `get_keys_vector`.
## v0.7.7 2024-08-28
Expand Down
20 changes: 9 additions & 11 deletions src/hdf5_hl.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ declare enum OBJECT_TYPE {
REFERENCE = "Reference",
REGION_REFERENCE = "RegionReference"
}
export declare type getReturnTypes = Dataset | Group | BrokenSoftLink | ExternalLink | Datatype | Reference | RegionReference;
export declare class BrokenSoftLink {
target: string;
type: OBJECT_TYPE;
Expand All @@ -50,13 +51,6 @@ export declare class ExternalLink {
type: OBJECT_TYPE;
constructor(filename: string, obj_path: string);
}
export declare class Datatype {
file_id: bigint;
path: string;
type: OBJECT_TYPE;
constructor(file_id: bigint, path: string);
get metadata(): Metadata;
}
export declare class Reference {
ref_data: Uint8Array;
constructor(ref_data: Uint8Array);
Expand Down Expand Up @@ -91,20 +85,24 @@ declare abstract class HasAttrs {
create_attribute(name: string, data: GuessableDataTypes, shape?: number[] | null, dtype?: string | null): void;
delete_attribute(name: string): number;
create_reference(): Reference;
dereference(ref: Reference | RegionReference): BrokenSoftLink | ExternalLink | Datatype | Group | Dataset | DatasetRegion | null;
dereference(ref: Reference | RegionReference): DatasetRegion | getReturnTypes | null;
}
export declare class Datatype extends HasAttrs {
constructor(file_id: bigint, path: string);
get metadata(): Metadata;
}
export declare class Group extends HasAttrs {
constructor(file_id: bigint, path: string);
keys(): Array<string>;
values(): Generator<BrokenSoftLink | ExternalLink | Datatype | Group | Dataset | null, void, unknown>;
items(): Generator<(string | BrokenSoftLink | ExternalLink | Datatype | Group | Dataset | null)[], void, unknown>;
values(): Generator<getReturnTypes | null, void, unknown>;
items(): Generator<(string | Reference | Dataset | Group | BrokenSoftLink | ExternalLink | Datatype | null)[], void, unknown>;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Returning a tuple should work and be more accurate:

Suggested change
items(): Generator<(string | Reference | Dataset | Group | BrokenSoftLink | ExternalLink | Datatype | null)[], void, unknown>;
items(): Generator<[string, getReturnTypes | null], void, unknown>;

Copy link
Member

Choose a reason for hiding this comment

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

This part is generated by the typescript compiler directly, not by me

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh right, because you changed the return type of Group#get. I think we can "fix" it by defining the return type explicitly on Group#items.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

get_type(obj_path: string): number;
get_link(obj_path: string): string;
get_external_link(obj_path: string): {
filename: string;
obj_path: string;
};
get(obj_name: string): BrokenSoftLink | ExternalLink | Datatype | Group | Dataset | null;
get(obj_name: string): getReturnTypes | null;
create_group(name: string, track_order?: boolean): Group;
create_dataset(args: {
name: string;
Expand Down
33 changes: 17 additions & 16 deletions src/hdf5_hl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -504,6 +504,8 @@ enum OBJECT_TYPE {
REGION_REFERENCE = 'RegionReference',
}

export type getReturnTypes = Dataset | Group | BrokenSoftLink | ExternalLink | Datatype | Reference | RegionReference;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perhaps the term Entity would be more suited? In H5Web, Entity is the base interface, and ProvidedEntity is the union of all entity interfaces that extend Entity, but since the bare term "entity" doesn't seem to be used yet in h5wasm, I see no problem using it. Otherwise, maybe ChildEntity?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I used Entity, but feel free to rename. I ruled out Object since it's already used by TypeScript.


export class BrokenSoftLink {
// only used for broken links...
target: string;
Expand All @@ -523,20 +525,6 @@ export class ExternalLink {
}
}

export class Datatype {
file_id: bigint;
path: string;
type: OBJECT_TYPE = OBJECT_TYPE.DATATYPE
constructor(file_id: bigint, path: string) {
this.file_id = file_id;
this.path = path;
}

get metadata() {
return Module.get_datatype_metadata(this.file_id, this.path);
}
}

export class Reference {
ref_data: Uint8Array;
constructor(ref_data: Uint8Array) {
Expand Down Expand Up @@ -677,14 +665,27 @@ abstract class HasAttrs {
return new Reference(ref_data);
}

dereference(ref: Reference | RegionReference) {
dereference(ref: Reference | RegionReference): DatasetRegion | getReturnTypes | null {
const is_region = (ref instanceof RegionReference);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Might be cleaner to override this method in the RegionReference class to avoid returning DatasetRegion when invoking this method on other kinds of objects.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

RegionReference doesn't extend HasAttrs, so what I suggested was not possible. Instead, I added signature overrides to the dereference method so that if one passes a RegionReference, the return type is DatasetRegion:

dereference(ref: RegionReference): DatasetRegion;
dereference(ref: Reference | RegionReference): DatasetRegion | getReturnTypes | null;

const name = Module.get_referenced_name(this.file_id, ref.ref_data, !is_region);
const target = this.root.get(name);
return (is_region) ? new DatasetRegion(target as Dataset, ref) : target;
}
}

export class Datatype extends HasAttrs {
constructor(file_id: bigint, path: string) {
super();
this.file_id = file_id;
this.path = path;
this.type = OBJECT_TYPE.DATATYPE;
}

get metadata() {
return Module.get_datatype_metadata(this.file_id, this.path);
}
}

export class Group extends HasAttrs {
constructor(file_id: bigint, path: string) {
super();
Expand Down Expand Up @@ -723,7 +724,7 @@ export class Group extends HasAttrs {
return Module.get_external_link(this.file_id, obj_path);
}

get(obj_name: string) {
get(obj_name: string): getReturnTypes | null {
let fullpath = (/^\//.test(obj_name)) ? obj_name : this.path + "/" + obj_name;
fullpath = normalizePath(fullpath);

Expand Down
4 changes: 4 additions & 0 deletions src/hdf5_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,10 @@ val get_attribute_names(hid_t loc_id, const std::string& obj_name_string)
{
ocpl_id = H5Gget_create_plist(obj_id);
}
else if (obj_type == H5O_TYPE_NAMED_DATATYPE)
{
ocpl_id = H5Tget_create_plist(obj_id);
}
else
{
ocpl_id = H5Dget_create_plist(obj_id);
Expand Down
Binary file modified test/array.h5
Binary file not shown.
Binary file modified test/compressed.h5
Binary file not shown.
4 changes: 4 additions & 0 deletions test/datatype_test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ async function datatype_test() {
littleEndian: false,
size: 10
});

assert.deepEqual(Object.keys(datatype.attrs), ['named_dtype_attr']);
assert.deepEqual(datatype.attrs['named_dtype_attr'].value,
'An attribute of a named datatype');
}

export const tests = [
Expand Down
1 change: 1 addition & 0 deletions test/make_test_files.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
f.create_dataset("bool", data=[[False, True], [True, False]], shape=(2,2))
f.create_dataset("bigint", data=np.arange(8).reshape(2,2,2), dtype="<i8", shape=(2,2,2))
f["datatype/value"] = np.dtype("S10")
f["datatype/value"].attrs["named_dtype_attr"] = "An attribute of a named datatype"

f.create_dataset("bigendian", data=[3,2,1], dtype='>f4')
f['bigendian'].attrs.create("bigendian_attr", [3,2,1], dtype='>i8')
Expand Down
Loading