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

Add set_data_types_from_keras_model() and set_accum_from_keras_model() for automatic precision inference #321

Open
wants to merge 50 commits into
base: main
Choose a base branch
from

Conversation

maksgraczyk
Copy link
Contributor

@maksgraczyk maksgraczyk commented Apr 14, 2021

Profiling information gathered e.g. by hls4ml.model.profiling.numerical() and other information can be used for setting data types in an HLSModel config heuristically so that the optimal precision configuration is achieved faster.

This PR does the following:

  • Adding set_data_types_from_keras_model() doing that with a Keras model.
  • Adding set_accum_from_keras_model() which sets accum_t of relevant layers based on a Keras model and data types set in an HLSModel config (unlike set_data_types_from_keras_model(), it doesn't use profiling information).
  • Adding extra arguments to config_from_keras_model() allowing for the functions above to be called automatically.

@thesps thesps self-requested a review April 19, 2021 09:37
@thesps
Copy link
Contributor

thesps commented Apr 20, 2021

The test failure is due to the new import from profiling that is introduced in utils. Specifically this line that imports matplotlib (but the pandas and seaborn import would fail too). You can see in setup.py that these packages are 'extras' for profiling and not automatically installed. We want to keep it this way so that users not using profiling can skip those packages.

For your PR, I think that just means we need to add some more try: import / except: ImportError guards around the packages only needed for making the plots (matplotlib, seaborn, pandas). The methods from profiling that you've called should work without those, and wrapping the imports that way should resolve the import error.

@maksgraczyk
Copy link
Contributor Author

If you want to familiarise yourself with the new method, you can have a look at this Jupyter notebook: https://gist.github.com/maksgraczyk/031ad1ec8191e237c2f5008adade2257

@maksgraczyk
Copy link
Contributor Author

I've made 2 extra commits after getting feedback from one of the users: the first one makes sure that only layers actually present in an HLSModel config are processed (it turns out that some Keras layers may not be actually present in an HLSModel config) and the second one allows set_data_types_from_keras_model() to be called also from hls4ml.utils rather than only hls4ml.utils.config.

@maksgraczyk maksgraczyk changed the title Add set_data_types_from_keras_model() for automatic precision inference Add set_data_types_from_keras_model() and set_accum_from_keras_model() for automatic precision inference Apr 23, 2021
min_b = 2 ** (type_b.integer - type_b.width)

max_val = n * max_w * max_i + max_b
min_val = min_b
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in the general case the minimum value could come either from the product of weight * input or the bias, depending on their precisions, not always from the bias.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bias may not exist (e.g., Dense(..., use_bias=False)), in that case we insert zeros and set precision to uint(1), which may throw this function off.

Copy link
Contributor Author

@maksgraczyk maksgraczyk Apr 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the feedback! I've just pushed the commits addressing both issues and adding Conv-type layer support.

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

Successfully merging this pull request may close these issues.

3 participants