search

chorus-ai / chorus_waveform

CHoRUS waveform documentation and various waveform conversion scripts
MIT License
3 stars 3 forks source link

zarr format updated to bit16int #99

Closed del42 closed 2 weeks ago

briangow commented 2 weeks ago

@del42 , thanks for this! In my test of this, it seems like you are getting fidelity errors that can't be explained simply from the change to lower precision 16 bit data:

Chunk           Numeric Samples               NaN Samples
    # Errors  /  Total    % Eq        SNR       NaN Values Match
Signal: II
  0       50556794/  51212896   1.281        1.8            N (7584)    
Subset of unuequal numeric data from input:
[-0.105 -0.1   -0.1   -0.105 -0.1   -0.09  -0.055  0.005  0.1    0.22 ]
Subset of unuequal numeric data from formatted file:
[0 0 0 0 0 0 0 0 0 0]

Also, if you make updates and it is convenient, could you separate out the open once, read many code and do a separate PR for that?

del42 commented 2 weeks ago

@del42 , thanks for this! In my test of this, it seems like you are getting fidelity errors that can't be explained simply from the change to lower precision 16 bit data:

Chunk         Numeric Samples               NaN Samples
  # Errors  /  Total    % Eq        SNR       NaN Values Match
Signal: II
  0     50556794/  51212896   1.281        1.8            N (7584)    
Subset of unuequal numeric data from input:
[-0.105 -0.1   -0.1   -0.105 -0.1   -0.09  -0.055  0.005  0.1    0.22 ]
Subset of unuequal numeric data from formatted file:
[0 0 0 0 0 0 0 0 0 0]

Also, if you make updates and it is convenient, could you separate out the open once, read many code and do a separate PR for that?

@briangow how are we handling NAN with bit16int, which doesn't represent NAN?

briangow commented 2 weeks ago

@briangow how are we handling NAN with bit16int, which doesn't represent NAN?

@del42 , I think you could store the NaNs as -32768 and then convert back after reading in the samples. This approach is fairly evident in the CCDEF code, if you want to use that as a template.

del42 commented 2 weeks ago

Also, if you make updates and it is convenient, could you separate out the open once, read many code and do a separate PR for that?

@briangow do I PR them same time or wait for one to merged?

briangow commented 2 weeks ago

@del42 , it's up to you, but if you the 16 bit code is ready, I'm happy to take a quick look and merge that one first.

briangow commented 2 weeks ago

@del42 , thanks for the update. The uncompressed version looks good now but the compressed version still has the fidelity check issue. Did you intend to add the gain adjustment to Zarr_compressed also?

del42 commented 2 weeks ago

@del42 , thanks for the update. The uncompressed version looks good now but the compressed version still has the fidelity check issue. Did you intend to add the gain adjustment to Zarr_compressed also?

Already added!

briangow commented 2 weeks ago

@del42 I don't think your switch to using fmt was fully implemented: AttributeError: 'Zarr' object has no attribute 'fmt'

briangow commented 2 weeks ago

This looks good in general, since it'll be easier to test and fix once the "open once, read many" code is implemented I'll merge as-is and the fmt issue can be fixed in another PR.

briangow commented 2 weeks ago

@del42 I don't think your switch to using fmt was fully implemented: AttributeError: 'Zarr' object has no attribute 'fmt'

I got this error because the Class names changed and I didn't realize that. This code in this PR is running fine as-is.