From 97ab658c11b35dec7ccd571be92d27fe67739c4e Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 19 Sep 2026 07:05:34 -0700 Subject: [PATCH] feat: attrs() reports every attribute; unsigned arrays stay unsigned MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit attrs() silently omitted any attribute whose datatype had no AttrValue variant — including every Python bool, which h5py stores as an enum — plus complex, compound and reference attributes, and cast unsigned 64-bit arrays to I64Array so values above i64::MAX came back negative. - numpy/h5py-style booleans (an enum of exactly FALSE=0 / TRUE=1 over an integer base) decode as I64 / I64Array of 0/1. - AttrValue::U64Array keeps unsigned arrays unsigned. Behaviour change: an unsigned array attribute no longer arrives as I64Array; the netCDF-4 CF helpers (_FillValue, valid_range) and the Python bindings handle it. - AttrValue::Raw { datatype, shape, data } carries any other attribute verbatim (also used when a value fails to decode as its declared type), so the attribute list is always complete. Decodable with data_read against the datatype; Python receives {"dtype", "shape", "data"}. - Both new variants are writable, so attributes round-trip between files. h5py interop tests cover reading 13 attribute kinds and h5py reading back a compound and a u64 attribute written by clawhdf5. Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 14 +++ crates/clawhdf5-format/src/type_builders.rs | 52 +++++++- crates/clawhdf5-netcdf4/src/cf.rs | 5 + crates/clawhdf5-py/src/lib.rs | 18 +++ crates/clawhdf5/src/types.rs | 61 +++++++-- crates/clawhdf5/tests/h5py_interop_tests.rs | 129 ++++++++++++++++++++ docs/known-issues.md | 18 ++- 7 files changed, 281 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 50f6397..adf75ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,20 @@ object_path }` (was `PathNotFound`), and a dataset whose raw data lives in external files (message 0x0007, now a known `MessageType`) is `ExternalDataFilesUnsupported` (it would otherwise read as fill values). +- **`attrs()` no longer drops attributes.** Any attribute whose datatype had + no `AttrValue` variant was omitted with no error — including every Python + `bool` (h5py stores `attrs["flag"] = True` as an enum), complex numbers, + compound values and object references. Now: + - numpy/h5py-style booleans (an enum of exactly `FALSE`=0 / `TRUE`=1) decode + as `I64` / `I64Array` of 0/1; + - new `AttrValue::U64Array` keeps unsigned arrays unsigned (they were cast to + `I64Array`, so values above `i64::MAX` came back negative). **Behaviour + change:** code matching `I64Array` for an unsigned attribute must also + match `U64Array` (the netCDF-4 CF helpers and Python bindings do); + - new `AttrValue::Raw { datatype, shape, data }` carries everything else + verbatim, decodable with `clawhdf5_format::data_read` against `datatype`. + Both new variants are writable, so an attribute can be copied between files + unchanged. Python receives `Raw` as `{"dtype", "shape", "data"}`. - All of the above are covered by h5py interop tests under both default and `libver='latest'` bounds, compared against h5py's own readback. diff --git a/crates/clawhdf5-format/src/type_builders.rs b/crates/clawhdf5-format/src/type_builders.rs index 4167b41..270cd48 100644 --- a/crates/clawhdf5-format/src/type_builders.rs +++ b/crates/clawhdf5-format/src/type_builders.rs @@ -279,6 +279,43 @@ pub(crate) fn build_attr_message(name: &str, value: &AttrValue) -> AttributeMess dataspace: scalar_ds(), raw_data: v.to_le_bytes().to_vec(), }, + AttrValue::U64Array(arr) => { + let mut raw = Vec::with_capacity(arr.len() * 8); + for v in arr { + raw.extend_from_slice(&v.to_le_bytes()); + } + AttributeMessage { + name: name.to_string(), + datatype: Datatype::FixedPoint { + size: 8, + byte_order: DatatypeByteOrder::LittleEndian, + signed: false, + bit_offset: 0, + bit_precision: 64, + }, + dataspace: simple_1d(arr.len() as u64), + raw_data: raw, + } + } + AttrValue::Raw { + datatype, + shape, + data, + } => AttributeMessage { + name: name.to_string(), + datatype: datatype.clone(), + dataspace: if shape.is_empty() { + scalar_ds() + } else { + Dataspace { + space_type: DataspaceType::Simple, + rank: shape.len() as u8, + dimensions: shape.clone(), + max_dimensions: None, + } + }, + raw_data: data.clone(), + }, AttrValue::String(s) => { let bytes = s.as_bytes(); AttributeMessage { @@ -334,7 +371,7 @@ pub(crate) fn simple_1d(n: u64) -> Dataspace { // ---- Attribute values ---- -/// Convenient attribute values for the write API. +/// Attribute values, for both the write API and what reading returns. #[derive(Debug, Clone)] pub enum AttrValue { F64(f64), @@ -342,8 +379,21 @@ pub enum AttrValue { I64(i64), I64Array(Vec), U64(u64), + /// Unsigned integers, kept unsigned so values above `i64::MAX` survive. + U64Array(Vec), String(String), StringArray(Vec), + /// An attribute whose datatype has no dedicated variant above (compound, + /// general enum, complex, reference, opaque, array, ...), carried verbatim + /// so it is never silently lost: the datatype, the dataspace dimensions + /// (empty for a scalar) and the element bytes exactly as stored. Decode + /// `data` with `clawhdf5_format::data_read` (e.g. `read_compound_fields`) + /// against `datatype`. Writing a `Raw` value stores it back unchanged. + Raw { + datatype: Datatype, + shape: Vec, + data: Vec, + }, } // ---- Dataset builder ---- diff --git a/crates/clawhdf5-netcdf4/src/cf.rs b/crates/clawhdf5-netcdf4/src/cf.rs index fa989c1..e66dee5 100644 --- a/crates/clawhdf5-netcdf4/src/cf.rs +++ b/crates/clawhdf5-netcdf4/src/cf.rs @@ -142,6 +142,7 @@ fn get_fill_value(attrs: &HashMap, key: &str) -> Option Some(FillValue::String(s.clone())), Some(AttrValue::F64Array(arr)) if !arr.is_empty() => Some(FillValue::Float(arr[0])), Some(AttrValue::I64Array(arr)) if !arr.is_empty() => Some(FillValue::Int(arr[0])), + Some(AttrValue::U64Array(arr)) if !arr.is_empty() => Some(FillValue::UInt(arr[0])), _ => None, } } @@ -155,6 +156,10 @@ fn get_valid_range(attrs: &HashMap) -> Option<(f64, f64)> { Some(AttrValue::I64Array(arr)) if arr.len() >= 2 => { return Some((arr[0] as f64, arr[1] as f64)); } + // Unsigned variables (NC_UBYTE..NC_UINT64) carry unsigned attributes. + Some(AttrValue::U64Array(arr)) if arr.len() >= 2 => { + return Some((arr[0] as f64, arr[1] as f64)); + } _ => {} } diff --git a/crates/clawhdf5-py/src/lib.rs b/crates/clawhdf5-py/src/lib.rs index c941386..5721964 100644 --- a/crates/clawhdf5-py/src/lib.rs +++ b/crates/clawhdf5-py/src/lib.rs @@ -128,10 +128,28 @@ pub(crate) fn attr_value_to_py(py: Python<'_>, val: &clawhdf5_rs::AttrValue) -> let list = pyo3::types::PyList::new(py, a).unwrap(); list.into_any().unbind() } + clawhdf5_rs::AttrValue::U64Array(a) => { + let list = pyo3::types::PyList::new(py, a).unwrap(); + list.into_any().unbind() + } clawhdf5_rs::AttrValue::StringArray(a) => { let list = pyo3::types::PyList::new(py, a).unwrap(); list.into_any().unbind() } + // No Python-side decoding for this datatype: hand back everything + // needed to interpret it rather than dropping the attribute. + clawhdf5_rs::AttrValue::Raw { + datatype, + shape, + data, + } => { + let dict = pyo3::types::PyDict::new(py); + dict.set_item("dtype", format!("{datatype:?}")).unwrap(); + dict.set_item("shape", shape).unwrap(); + dict.set_item("data", pyo3::types::PyBytes::new(py, data)) + .unwrap(); + dict.into_any().unbind() + } } } diff --git a/crates/clawhdf5/src/types.rs b/crates/clawhdf5/src/types.rs index e1743a6..91cb884 100644 --- a/crates/clawhdf5/src/types.rs +++ b/crates/clawhdf5/src/types.rs @@ -163,13 +163,53 @@ pub(crate) fn attrs_to_map( ) -> HashMap { let mut map = HashMap::new(); for attr in attrs { - if let Some(val) = decode_attr_value(attr, file_data, offset_size, length_size) { - map.insert(attr.name.clone(), val); - } + // Every attribute is reported. One that has no dedicated `AttrValue` + // variant, or that fails to decode as its declared type, is returned + // verbatim as `AttrValue::Raw` rather than dropped — a partial + // attribute list with no indication anything is missing is worse than + // an undecoded value. + let val = + decode_attr_value(attr, file_data, offset_size, length_size).unwrap_or_else(|| { + AttrValue::Raw { + datatype: attr.datatype.clone(), + shape: attr.dataspace.dimensions.clone(), + data: attr.raw_data.clone(), + } + }); + map.insert(attr.name.clone(), val); } map } +/// `Some(values)` if `attr` is a numpy/h5py-style boolean: an enumeration over +/// an integer base type whose members are exactly `FALSE` = 0 and `TRUE` = 1. +/// This is how `attrs["flag"] = True` is stored. Reported as 0/1 integers. +fn decode_bool_enum(attr: &clawhdf5_format::attribute::AttributeMessage) -> Option> { + use clawhdf5_format::datatype::Datatype; + + let Datatype::Enumeration { + base_type, members, .. + } = &attr.datatype + else { + return None; + }; + if members.len() != 2 { + return None; + } + let member_value = |name: &str| { + let m = members.iter().find(|m| m.name.eq_ignore_ascii_case(name))?; + clawhdf5_format::data_read::read_as_i64(&m.value, base_type) + .ok()? + .first() + .copied() + }; + if member_value("FALSE")? != 0 || member_value("TRUE")? != 1 { + return None; + } + let values = clawhdf5_format::data_read::read_as_i64(&attr.raw_data, base_type).ok()?; + values.iter().all(|v| *v == 0 || *v == 1).then_some(values) +} + fn decode_attr_value( attr: &clawhdf5_format::attribute::AttributeMessage, file_data: &[u8], @@ -200,12 +240,7 @@ fn decode_attr_value( if vals.len() == 1 { Some(AttrValue::U64(vals[0])) } else { - // No U64Array variant, store as I64Array. - // NOTE: This cast is lossy for values > i64::MAX (bit 63 set). - // Those values will appear as negative i64. A dedicated U64Array - // variant would be needed to handle the full u64 range. - let i64_vals: Vec = vals.iter().map(|&v| v as i64).collect(); - Some(AttrValue::I64Array(i64_vals)) + Some(AttrValue::U64Array(vals)) } } Datatype::String { .. } => { @@ -228,6 +263,14 @@ fn decode_attr_value( Some(AttrValue::StringArray(strings)) } } + Datatype::Enumeration { .. } => { + let vals = decode_bool_enum(attr)?; + if vals.len() == 1 { + Some(AttrValue::I64(vals[0])) + } else { + Some(AttrValue::I64Array(vals)) + } + } _ => None, } } diff --git a/crates/clawhdf5/tests/h5py_interop_tests.rs b/crates/clawhdf5/tests/h5py_interop_tests.rs index 83becfb..2402a07 100644 --- a/crates/clawhdf5/tests/h5py_interop_tests.rs +++ b/crates/clawhdf5/tests/h5py_interop_tests.rs @@ -784,3 +784,132 @@ with h5py.File("links_{tag}.h5", "w"{kwargs}) as f: ); } } + +// --------------------------------------------------------------------------- +// Attribute fidelity: nothing is dropped, unsigned stays unsigned +// --------------------------------------------------------------------------- + +/// `attrs()` used to omit, without any error, every attribute whose datatype +/// had no `AttrValue` variant — including every Python `bool` (stored as an +/// enum) — and to wrap unsigned 64-bit arrays into negative `i64`s. +#[test] +fn h5py_attribute_kinds_are_all_reported() { + skip_if_no_python!(); + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("attrs_kinds.h5"); + let path_str = path.display().to_string(); + let script = format!( + r#" +import h5py, numpy as np +with h5py.File("{path_str}", "w") as f: + d = f.create_dataset("d", data=np.arange(3)) + a = d.attrs + a["float"] = 1.5; a["int"] = 7; a["str"] = "hello"; a["int_array"] = np.arange(4) + a["str_list"] = ["a", "bc"] + a["uint64_array"] = np.array([2**63, 1], dtype="u8") + a["bool_true"] = True + a["bool_false"] = False + a["bool_array"] = np.array([True, False, True]) + a["complex"] = 1 + 2j + a["compound"] = np.array([(1, 2.5)], dtype=[("a", " { + assert_eq!(data, &[2]); + assert!(format!("{datatype:?}").contains("BLUE")); + } + other => panic!("color: {other:?}"), + } + // A compound attribute is kept verbatim and decodes against its datatype. + match &attrs["compound"] { + AttrValue::Raw { + datatype, + shape, + data, + } => { + assert_eq!(shape, &[1]); + let fields = clawhdf5_format::data_read::read_compound_fields(data, datatype).unwrap(); + assert_eq!(fields[0].name, "a"); + let b = + clawhdf5_format::data_read::read_as_f64(&fields[1].raw_data, &fields[1].datatype) + .unwrap(); + assert_eq!(b, vec![2.5]); + } + other => panic!("compound: {other:?}"), + } + for name in ["complex", "object_ref"] { + assert!( + matches!(attrs[name], AttrValue::Raw { .. }), + "{name}: {:?}", + attrs[name] + ); + } +} + +/// `Raw` and `U64Array` are writable, so an attribute read from one file can +/// be stored in another unchanged. +#[test] +fn clawhdf5_writes_raw_and_unsigned_attrs_h5py_reads() { + skip_if_no_python!(); + let dir = tempfile::tempdir().unwrap(); + let src = dir.path().join("src.h5"); + let dst = dir.path().join("dst.h5"); + let (src_str, dst_str) = (src.display().to_string(), dst.display().to_string()); + run_python(&format!( + r#" +import h5py, numpy as np +with h5py.File("{src_str}", "w") as f: + d = f.create_dataset("d", data=np.arange(3)) + d.attrs["compound"] = np.array([(1, 2.5), (3, 4.5)], dtype=[("a", "