Skip to content

Support for HDF5 file read/write in usl_lib#77

Open
rsutormin wants to merge 4 commits intomainfrom
rs-flood-hdf5
Open

Support for HDF5 file read/write in usl_lib#77
rsutormin wants to merge 4 commits intomainfrom
rs-flood-hdf5

Conversation

@rsutormin
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Contributor

@waltaskew waltaskew left a comment

Choose a reason for hiding this comment

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

Do we need this interface? The h5py interface already exposes hdf5 files as dictionary-like objects.

Comment thread usl_pipeline/usl_lib/usl_lib/readers/hdf5_readers.py Outdated
group = hf.get(group_name)
layers: Dict[str, npt.NDArray[numpy.float32]] = {}
for key, layer in group.items():
layers[key] = numpy.array(layer, dtype=numpy.float32)
Copy link
Copy Markdown
Contributor

@waltaskew waltaskew Jun 17, 2024

Choose a reason for hiding this comment

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

As above, we won't want to convert the masks to floats. I'd just leave them as-is and assume they've been written with the correct dtype.

I think the idiom for this is [:]

layers[key] = layer[:]

Also, do we need to create this new dictionary? Can we just work with hf.get(group_name), which is already a dict-like object?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I checked, when you return group itself it becomes unconnected from the data after the h5py.File object gets closed. So this conversion to Dict of Numpy is needed for later usage.

Comment thread usl_pipeline/usl_lib/usl_lib/writers/hdf5_writers.py Outdated
Comment thread usl_pipeline/usl_lib/usl_lib/readers/hdf5_readers.py Outdated
@rsutormin
Copy link
Copy Markdown
Contributor Author

Do we need this interface? The h5py interface already exposes hdf5 files as dictionary-like objects.

I would prefer this to be a one-liner in the giant CF/main.py file (btw, I would vote for decomposing it at least into flood and wrf parts).

Copy link
Copy Markdown
Contributor

@waltaskew waltaskew left a comment

Choose a reason for hiding this comment

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

This all LGTM, but I'd like to let Ahmed work on this & get experience with things like managing requirements.txt

@waltaskew waltaskew removed their assignment Jun 25, 2024
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