hls4ml
hls4ml copied to clipboard
fix nondefault project name handling
Description
As discussed in #624, convert_from_keras_model with project_name set to nondefault value (e.g. mynewprojname) fails to build, because in build.tcl the variable ${mynewprojname} is not defined. This fixes the problem my always leaving the variable called myproject
, but just changing the value of the variable. Some changes in the tcl code were requried since now since the variable name does not always match the value it contains. The build.tcl indentation was fixed, and the parsing of the Vivado report was fixed. Note that this is broken in main even in the default case currently.
Type of change
- [x] Bug fix (non-breaking change that fixes an issue)
Tests
Testing was done offline by modifying and running part1 of the hls4ml-tutorial, as mentioned in #624. Nevertheless, we probably should add a test. The complication is that the test requires synthesis, and generally our pytests don't do synthesis. Plus, synthesis is long. Do we want to add another synthesis test for this case or modify an existing one?
Checklist
- [x] I have read the guidelines for contributing.
- [x] I have commented my code, particularly in hard-to-understand areas.
- [x] I have made corresponding changes to the documentation.
- [x] My changes generate no new warnings.
- [ ] I have added tests that prove my fix is effective or that my feature works.
The qkeras pytest failure is a known problem that exists in the main branch even without this change.
Thanks for the fix, I don't think we need a new test for this
The last push was not a complete fix. I'll send a message again when I think it's ready.
I still need to test vsynth=True and also that I didn't mess up the FIFO optimizations.
Actually, looking in the main branch as a starting point, I see
'VivadoSynthReport': {'LUT': '0',
'FF': '0',
'BRAM_18K': '0',
'URAM': '0',
'DSP48E': '0'}
when I do part1 of the hls4ml-tutorial with hls_model.build(vsynth=True)
so vsynth might be broken in the main branch. Will try to investigate.
This has been broken for some time now, I observed it as well while playing with Vitis. Thanks for looking into it.
This seems to break the VivadoAccelerator backend.
I believe this is ready for merging.