[Feature][QDP] Pipeline file load respects PipelineConfig.dtype#1407
Conversation
d27c747 to
ffc84e1
Compare
|
cc @rich7420 |
There was a problem hiding this comment.
overall lg. The dtype wiring looks like the right direction overall, but basis should not go through a lossy f32 representation before index validation.
I also left a smaller inline note about the Python loader defaulting file pipelines to f32. If that default is intentional, it probably needs to be explicit in the public API/docs.
…/0lai0/mahout into feature-1341-pipeline-file-dtype
| use crate::reader::DataReader; | ||
| let mut reader = crate::readers::ParquetReader::new(path, None, null_handling)?; | ||
| reader.read_batch() | ||
| if matches!(dtype, Precision::Float32) { |
There was a problem hiding this comment.
One concern here: this f32 path also covers basis, where the values are indices rather than ordinary float features. A valid index like 16_777_217 for 25 qubits is not exactly representable as f32, so it becomes 16_777_216; the basis f32 encoder then validates the rounded value and encodes the wrong state. Can we keep basis file input in f64/integer form, or reject f32 basis input once indices exceed the exact f32 range?
There was a problem hiding this comment.
Thanks @ryankert01 and @rich7420 for review. Sure, I'll keep basis file input in f64 and reject an explicit dtype=f32 request in next commit.
| dtype: Precision, | ||
| fmt: &str, | ||
| ) -> (BatchData, usize, usize) { | ||
| if matches!(dtype, Precision::Float32) { |
There was a problem hiding this comment.
This cast now affects QuantumDataLoader.source_file() too, because the Python loader factories build file pipelines with Dtype::Float32 and there is no public loader dtype option. That makes f64 file input narrow by default for f32-capable encodings. Is that the behavior we want? If so, I think it should be called out in the loader docs, and ideally exposed as an explicit loader setting.
There was a problem hiding this comment.
Great catch. I've updated the implementation so that file loads now default to f64 for lossless precision, while Float32 is now opt-in via a new public .dtype() loader method, where parse_dtype(None) defaults to Float64. Thanks for review.
| let _ = fs::remove_file(&path); | ||
| let (batch_data, num_samples, sample_size) = result.unwrap(); | ||
| assert!( | ||
| matches!(batch_data, BatchData::F32(_)), |
There was a problem hiding this comment.
nit (non-blocking): these new tests only check the BatchData variant, not the values — so a batch that comes back as F32 but with zeroed/garbled contents would still pass green. Might be worth reading one element back (e.g. assert the first value ≈ 0.25) so it's a real check, like test_synthetic_producer_f32_amplitude does. Same gap means the cast_f64_to_batch_data path (arrow/npy/pt/pb → f32) and the f64-parquet→f32 cross-cast don't have coverage yet.
There was a problem hiding this comment.
Tests now assert the actual values (first element ≈ 0.25), not just the variant. Thanks for review.
| let prefetch_depth = config.prefetch_depth; | ||
| let (rx, recycle_tx, _producer_handle) = spawn_producer(producer, prefetch_depth)?; | ||
| let (rx, recycle_tx, _producer_handle) = match batch_data { | ||
| BatchData::F32(data) => spawn_producer( |
There was a problem hiding this comment.
nit (totally optional): these two arms are byte-for-byte identical apart from the <f32>/<f64> type param, and the streaming path already has build_streaming_producer<T> — a little build_inmemory_producer<T> helper here would kill the copy-paste and mirror that pattern. Just caught my eye while reading.
There was a problem hiding this comment.
I updated extracted::build_inmemory_producer<T> so that the two from_file arms are now consolidated into a single generic call, aligning it with build_streaming_producer<T>.
|
btw, @0lai0 thanks for your patch! |
|
@ryankert01 thanks for the review , @0lai0 thanks for the patch! |
3 participants
Related Issues
Closes #1341
Part of #1338
Changes
Why
PipelineConfig.dtypeis silently ignored when loading from files.read_file_by_extensionalways returnedVec<f64>, soBatchData::F64was always produced andencode_batch_f32_for_pipelinewas never reached from a file source.ParquetReader<T> / ParquetStreamingReader<T>already had f32/f64 support from #1393 (issue #1340) — this PR just wires the dtype through.How
T: FloatElem (default f64). A pub(crate) ToBatchData trait handles wrapping/unwrapping BatchData inside produce().Checklist