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

waveform_hdrgen.py: Two minors bugs (mode filter and temperature range) and one proposal to support non 5 bpp waveforms in JSON #377

Open
dapeda42 opened this issue Nov 22, 2024 · 1 comment

Comments

@dapeda42
Copy link

dapeda42 commented Nov 22, 2024

I think there are two minor bugs in waveform_hdrgen.py:

1. Bug: Default mode_filter is wrong
If no export_modes are given, the default mode_filter is set in line 64 as follows:
mode_filter = list(range(len(waveforms["modes"])))

With 5 modes in waveform, mode_filter is 0,1,2,3,4
However, mode_filter should list the exctual modes given in the waveform.
For example, if the waveform contains five modes: 1, 2, 5, 16, and 17 (like ED097OC4.json, generated with epdiy_waveform_gen.py), then mode_filter should be 1,2,5,16,17

This can be achieved by changing line 64 as follows:
mode_filter = [wm["mode"] for wm in waveforms["modes"]]

2. Bug: Temperature interval is too long if temperature range is limited
For example, the waveform contains five temperature ranges: 10-15, 15-20, 20-25, 25-30, and 30-35
If argument '--temperature-range' is set to '16,26' (i.e., tmin=16 and tmax=26), then
a) Waveforms (EpdWaveformPhases) are exported to the header file for the three requested temperature ranges only (15-20, 20-25, 25-30). This is ok.

b) ALL temperature ranges are exported, i.e., EpdWaveformTempInterval contains ALL five temperature ranges (10-15, 15-20, 20-25, 25-30, 30-35). This is presumably NOT ok.

In waveform_hdrgen.py the temperature ranges are created in lines 74 and 75:

for bounds in waveforms["temperature_ranges"]["range_bounds"]:
    temp_intervals.append(f"{{ .min = {bounds['from']}, .max = {bounds['to']} }}")

This code MUST consider tmin and tmax (like in lines 87 and 88):

for bounds in waveforms["temperature_ranges"]["range_bounds"]:
    if bounds["from"] < tmin or bounds["from"] > tmax:
        continue
    temp_intervals.append(f"{{ .min = {bounds['from']}, .max = {bounds['to']} }}")

3. Proposal to support non 5 bits per pixel waveforms in JSON
The current version of phase_to_c (function in waveform_hdrgen.py) supports 5 bits per pixel (bpp) waveforms in the JSON file only and gives 4 bpp waveforms in the header file.
To relax these requirements (e.g., support JSON files with 4 bpp waveforms), I propose the following version of phase_to_c:

def phase_to_c(phase,bits_per_pixel_c=4):
    global total_size
    
    N1 = len(phase)
    N2 = len(phase[0])
    N = 2**bits_per_pixel_c
    
    if N1%N != 0:
        raise ValueError(f"First dimension of phases is {N1}. Allowed are multiples of {N}")
    
    step1 = int(N1/N)
        
    if N2%N != 0:
        raise ValueError(f"Second dimension of phases is {N2}. Allowed are multiples of {N}")
    
    step2 = int(N2/N)

    targets = []
    for t in range(0, N1, step1):
        chunk = 0
        line = []
        i = 0
        for f in range(0, N2, step2):
            fr = phase[t][f]
            chunk = (chunk << 2) | fr
            i += 1
            if i == 4:
                i = 0
                line.append(chunk)
                chunk = 0
        targets.append(line)
        total_size += len(line)

    return targets
@vroland
Copy link
Owner

vroland commented Nov 22, 2024

I haven't looked at it in a while, but this seems plausible. Could you do a pull request with your changes?

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

No branches or pull requests

2 participants