-
Notifications
You must be signed in to change notification settings - Fork 409
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
GarNet and GarNetStack in config.py #344
GarNet and GarNetStack in config.py #344
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yiiyama Do you have an example of a realistic HLSConfig
you would use?
Here's my naive attempt at a conversion script, which fails due to #365.
Also, as noted in #365, I'm wondering if we can solve this so HLSModel.compile()
and predict()
work. I think @vloncar said he had an idea for a workaround in case we can't include hls_math.h
due to copyright issues.
I also wonder if output_activation='linear'
can also be allowed here in addition to None
(Naively, that's what I tried at first when I saw the error message).
This is the configuration file used for the HLS GarNet paper. Basically you are better off not setting too many options. In particular, as I write in the PR intro above, the config dict returned by config_from_keras_model for GarNet is actually suboptimal - it just creates entries for all available tuning parameters and sets default values for them, while the GarNet class in hls_layers.py can actually compute the optimal precision for the weight parameters given an actual model.
This and this are my attempts at solving the issue. @vloncar what do you think?
That's a good point; 'linear' is allowed now. I tweaked your gist a bit here. The only differences are
With the current HEAD of this PR branch, the script does execute and ends up with synched-ish result. |
Re: #365 (comment) I wanted to check how our python API result differs from csim/cosim. I ran this updated gist: https://gist.github.com/jmduarte/c1eedff75fc941e755ae82f7d626093f Partial good news is python API and csim seem to be similar up to some truncation effects? Bad news is cosim currently fails.
|
Also, now that testing is merged, I wonder if we can add a test similar to the gist? Now done in 3adc72c |
42bfea8
to
22420a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now! Batched inputs should also work with #414
83d6554
to
81d6b42
Compare
…garnet GarNet and GarNetStack in config.py
I was asked by a Ph.D. student at Imperial College London why
hls4ml config
doesn't work with GarNet. Found out that graph_layers support isn't there in config.py - which is of course the case because I didn't do it (hls4ml config wasn't really a thing when I was working on GarNet).This PR adds support to GarNet and GarNetStack in config.py and gives some default precision values for their parameters. These default values are however most likely not optimal, because in config.py we don't deal with tensor shapes and therefore we don't know the number of vertices that goes into the layer, which is a critical input when computing optimal precisions.
So the lines that gets generated with
hls4ml config
are there mostly for documentation purpose to say "hey these are the configurable parameters", and users are actually better off commenting them out when actually doing a conversion (proper default precisions will be calculated using tensor shapes), unless they really have specific precision values they want to use. Is this something to be documented somewhere? (add a comment line in the generated config?)