fastmachinelearning / hls4ml

Machine learning on FPGAs using HLS
https://fastmachinelearning.org/hls4ml
Apache License 2.0
1.24k stars 402 forks source link

Change fractional (and others) to be a property, move quantizers #964

Closed jmitrevs closed 7 months ago

jmitrevs commented 7 months ago

Description

This PR has two somewhat unrelated changes. I made the second after the first, so I don't have the second standalone (but can if necessary), and it's easy to extract just the first since it's the first commit. The changes are:

  1. Currently FixedPrecisionType and IntegerPrecisionType have the values width, integer, and fractional set as values stored in the classes. Though they are set properly initially, the values are mutable, so it's possible to edit the values after the fact. It's up to whoever modifies them to make sure they remain consistent. I think that's error-prone. For example, one could modify width in IntegerPrecisionType but leave integer alone, leaving a malformed class. What this PR does is make fractional a property of FixedPrecisonType, always returning width - integer, and a property of IngegerPrecisionType, always returning 0. Similarly integer is a property of IntegerPrecisionType, just returning width. They don't have setters. I also modified the saturation and rounding properties to return the default value if they are set to None, and added similar properties to IntegerPrecisionType but unchangeable, since they are not changeable in the type.
  2. Move quantizers out of types.py into a new quantizers.py file. For qonnx we will add additional quantizers, so my thinking is that it's better to have them in a separate file. But this is not a major issue, so I would be fine reverting it if there are objections.

Type of change

Tests

There should be no changes so the old tests should confirm that I didn't introduce an error.

Checklist

jmitrevs commented 7 months ago

I think this looks good. Since I initially submitted this PR I can't approve it, but if you are happy with it, go ahead and merge it.