Skip to content

Conversation

calad0i
Copy link
Contributor

@calad0i calad0i commented Sep 9, 2025

Description

Remove dependency of #1139 on fxpmath, which generates a warning when pip installing for using outdated setup.cfg.

Type of change

  • Housekeeping

Tests

Inherits from #1139

Checklist

  • all

@calad0i calad0i added the please test Trigger testing by creating local PR branch label Sep 9, 2025
@calad0i
Copy link
Contributor Author

calad0i commented Sep 9, 2025

Ok, we have two tests fails:
How many bits shall be there for -1024 and -0.03125? The current test asks for 2 for both, but technically, one can fit them in ap_fixed<1,11> and ap_fixed<1,-4>...

For the oneapi flavored ac, signed numbers (actually, all for now) needs at least 2 bits, but that is a separate issue and is addressed by #1334.

@jmitrevs
Copy link
Contributor

jmitrevs commented Sep 9, 2025

I think I made negative values always have at least 2 bits. I figured for the use cases it was simpler with little effect. I am not opposed to changing it to be backend-specific provided it breaks nothing.

@calad0i
Copy link
Contributor Author

calad0i commented Sep 9, 2025

By definition of the fixed point numbers, there will always be one more value on the negative side, and ac_fixed has the same behavior here. The problem is likely oneapi specific. Also, for the current oneapi to work, unsigned numbers also needs >=2 bits iirc.

@jmitrevs
Copy link
Contributor

jmitrevs commented Sep 9, 2025

I had assumed it was just signed that required two bits, but I might be wrong. Best to confirm with @laurilaatu .

@laurilaatu
Copy link
Contributor

laurilaatu commented Sep 10, 2025

@jmitrevs @calad0i
Currently 2 bits is required for both signed and unsigned cases for ac_fixed. For ap_fixed both cases can be 1 bit.

@jmitrevs
Copy link
Contributor

Should we make 2 bits the minimum in all cases for now? This is only used for constants in threshold relu and such, so 1 vs 2 bits should be pretty insignificant.

@calad0i
Copy link
Contributor Author

calad0i commented Sep 10, 2025

For ac_fixed it is already the case (see https://github.com/fastmachinelearning/hls4ml/blob/main/hls4ml/backends/fpga/fpga_types.py#L99). For ap_fixed there is no need to do this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
please test Trigger testing by creating local PR branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants