✨ Add SyReC synthesis with Additional Lines #65

SmaranTum · 2022-08-24T11:33:24Z

This PR implements the original SyReC synthesis approach using additional lines. The syrec-editor GUI is updated accordingly.

…syrec.cpp/hpp) Included the corresponding python bindings (bindings.cpp) Added the corresponding tests (test_add_lines_synthesis.cpp, test_add_lines_simulation) Included corresponding json files required for testing purpose (circuit_simulation_add_lines.cpp, circuit_synthesis_add_lines.cpp) Included the corresponding python tests (test_syrec.cpp) Made necessary minor changes to the respective cmake files.

codecov · 2022-08-24T11:43:12Z

Codecov Report

Merging #65 (acbd748) into main (f898102) will decrease coverage by 0.2%.
The diff coverage is 94.9%.

@@           Coverage Diff           @@
##            main     #65     +/-   ##
=======================================
- Coverage   91.5%   91.2%   -0.3%     
=======================================
  Files         25      29      +4     
  Lines       1789    1825     +36     
=======================================
+ Hits        1638    1666     +28     
- Misses       151     159      +8

Impacted Files	Coverage Δ
include/core/circuit.hpp	`76.1% <ø> (ø)`
include/core/gate.hpp	`20.0% <ø> (ø)`
include/core/properties.hpp	`50.0% <ø> (ø)`
include/core/syrec/expression.hpp	`100.0% <ø> (ø)`
include/core/syrec/grammar.hpp	`95.8% <ø> (ø)`
include/core/syrec/module.hpp	`90.9% <ø> (ø)`
include/core/syrec/number.hpp	`75.0% <ø> (ø)`
include/core/syrec/parser.hpp	`100.0% <ø> (ø)`
include/core/syrec/program.hpp	`100.0% <ø> (ø)`
include/core/syrec/statement.hpp	`95.3% <ø> (ø)`
... and 11 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

burgholzer

I just quickly went over all the changes here and as of now, this is not ready to be merged.
Please avoid code duplication at (almost) all cost. This tiny PR alone adds 1.8K lines of code to the repository of which most are duplicate code. For details, see the comments below.

include/algorithms/synthesis/syrec_synthesis.hpp

mqt/syrec/bindings.cpp

mqt/syrec/syrec_editor.py

src/algorithms/synthesis/syrec_synthesis.cpp

test/unittests/test_simulation.cpp

mqt/syrec/syrec_editor.py

…n a single file. Updated the GUI as required.

…r' into Adding-Original-SyReC-synthesizer

burgholzer

This looks much better already! Thanks.
Still, i believe there is room for improvement and some easily avoidable code duplication remains. You can find the detailed comments below.

include/algorithms/synthesis/syrec_synthesis.hpp

src/algorithms/synthesis/syrec_synthesis.cpp

mqt/syrec/syrec_editor.py

…s) to the syrec_synthesis.cpp/hpp file. derived class put in a seperate file.

…r' into Adding-Original-SyReC-synthesizer

Signed-off-by: Lukas Burgholzer <lukas.burgholzer@jku.at>

burgholzer

The last round of changes didn't go the direction I would have hoped. In particular

it hardly reduced code duplication
it did not split up the individual classes
it introduced different names for the same functions.

What I had in mind, was proper object oriented programming. I just pushed some commits that refactor the relevant classes in order to reduce the code duplication as much as possible.
Note how the additional lines synthesiser only defines 4 functions and that is it. Everything can be handled is handled in the base class.
This was made possible by just 1-2 small changes in some functions.
Overall the refactoring reduced the additional lines in this PR by roughly 300 lines.

All tests still pass. But please make sure that everything besides that still works as intended.

include/algorithms/synthesis/syrec_synthesis.hpp

src/algorithms/synthesis/syrec_synthesis_additional_lines.cpp

SmaranTum · 2022-09-05T13:57:48Z

The last round of changes didn't go the direction I would have hoped. In particular
* it hardly reduced code duplication

* it did not split up the individual classes

* it introduced different names for the same functions.
What I had in mind, was proper object oriented programming. I just pushed some commits that refactor the relevant classes in order to reduce the code duplication as much as possible. Note how the additional lines synthesiser only defines 4 functions and that is it. Everything can be handled is handled in the base class. This was made possible by just 1-2 small changes in some functions. Overall the refactoring reduced the additional lines in this PR by roughly 300 lines.

All tests still pass. But please make sure that everything besides that still works as intended.

Thank you very much for this. I didn't know that I had to do this. Again, learnt a lot from this

include/algorithms/synthesis/syrec_synthesis_additional_lines.hpp

burgholzer · 2022-09-05T14:32:58Z

I just resolved all the conflicts so that CI starts properly.

SmaranTum · 2022-09-05T15:22:01Z

Don't know why the CI is failing?

burgholzer · 2022-09-05T15:34:02Z

Don't know why the CI is failing?

Seems to just be a sporadic failure. The upload of the codecov report sometimes fails for whatever reason. Usually a restart of the job is enough to fix this.

burgholzer

This also looks great now. I'll again update the PR title and description and then merge the PR once CI passes.

Small side note: once this is merged, please wait a bit before creating the next PR. I am planning on introducing some more code quality features and improve some aspects of the python packaging during the rest of the day. I'll keep you posted (and you'll probably see the PRs)

SmaranTum · 2022-09-05T15:43:02Z

This also looks great now. I'll again update the PR title and description and then merge the PR once CI passes.

Small side note: once this is merged, please wait a bit before creating the next PR. I am planning on introducing some more code quality features and improve some aspects of the python packaging during the rest of the day. I'll keep you posted (and you'll probably see the PRs)

I have already created the dd synthesis PR. is that going to be an issue?

burgholzer · 2022-09-05T15:52:58Z

This also looks great now. I'll again update the PR title and description and then merge the PR once CI passes.
Small side note: once this is merged, please wait a bit before creating the next PR. I am planning on introducing some more code quality features and improve some aspects of the python packaging during the rest of the day. I'll keep you posted (and you'll probably see the PRs)

This also looks great now. I'll again update the PR title and description and then merge the PR once CI passes.
Small side note: once this is merged, please wait a bit before creating the next PR. I am planning on introducing some more code quality features and improve some aspects of the python packaging during the rest of the day. I'll keep you posted (and you'll probably see the PRs)

I have already created the dd synthesis PR. is that going to be an issue?

Most certainly not, but you'll have to deal with the merge conflicts 🙃

Merge branch 'cda-tum:main' into Adding-Original-SyReC-synthesizer

0fdf148

burgholzer requested changes Aug 29, 2022

View reviewed changes

SmaranTum added 2 commits August 30, 2022 22:52

Made necessary changes to include the additional line functionality i…

1c9fbfc

…n a single file. Updated the GUI as required.

Merge remote-tracking branch 'origin/Adding-Original-SyReC-synthesize…

1751a3e

…r' into Adding-Original-SyReC-synthesizer

burgholzer requested changes Sep 2, 2022

View reviewed changes

SmaranTum and others added 3 commits September 2, 2022 17:31

Merge branch 'cda-tum:main' into Adding-Original-SyReC-synthesizer

13d4c0b

Made the required changes (renamed functions to avoid virtual warning…

288a138

…s) to the syrec_synthesis.cpp/hpp file. derived class put in a seperate file.

Merge remote-tracking branch 'origin/Adding-Original-SyReC-synthesize…

baf86fc

…r' into Adding-Original-SyReC-synthesizer

SmaranTum requested a review from burgholzer September 3, 2022 11:21

burgholzer added 3 commits September 4, 2022 21:21

♻️refactor to reduce code duplication

8d483d6

Signed-off-by: Lukas Burgholzer <lukas.burgholzer@jku.at>

🐛 fix CMake

cfffaf0

Signed-off-by: Lukas Burgholzer <lukas.burgholzer@jku.at>

🔥remove unused import

52775a5

Signed-off-by: Lukas Burgholzer <lukas.burgholzer@jku.at>

burgholzer requested changes Sep 4, 2022

View reviewed changes

include/algorithms/synthesis/syrec_synthesis.hpp Show resolved Hide resolved

src/algorithms/synthesis/syrec_synthesis_additional_lines.cpp Outdated Show resolved Hide resolved

src/algorithms/synthesis/syrec_synthesis_additional_lines.cpp Outdated Show resolved Hide resolved

Reduce code duplication

d3dd159

burgholzer reviewed Sep 5, 2022

View reviewed changes

include/algorithms/synthesis/syrec_synthesis_additional_lines.hpp Outdated Show resolved Hide resolved

Merge branch 'main' into Adding-Original-SyReC-synthesizer

5aab0c0

Moved trival functions to the base class

df5f02d

Merge branch 'cda-tum:main' into Adding-Original-SyReC-synthesizer

acbd748

SmaranTum requested a review from burgholzer September 5, 2022 15:40

burgholzer approved these changes Sep 5, 2022

View reviewed changes

burgholzer changed the title ~~Included the original syrec functionality with additional lines~~ ✨ Add SyReC synthesis with Additional Lines Sep 5, 2022

burgholzer added the feature New feature or request label Sep 5, 2022

burgholzer enabled auto-merge (squash) September 5, 2022 15:44

burgholzer merged commit f1e591a into cda-tum:main Sep 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add SyReC synthesis with Additional Lines #65

✨ Add SyReC synthesis with Additional Lines #65

SmaranTum commented Aug 24, 2022 •

edited by burgholzer

Loading

codecov bot commented Aug 24, 2022 •

edited

Loading

burgholzer left a comment

burgholzer left a comment

burgholzer left a comment

SmaranTum commented Sep 5, 2022

burgholzer commented Sep 5, 2022

SmaranTum commented Sep 5, 2022

burgholzer commented Sep 5, 2022

burgholzer left a comment

SmaranTum commented Sep 5, 2022

burgholzer commented Sep 5, 2022

✨ Add SyReC synthesis with Additional Lines #65

✨ Add SyReC synthesis with Additional Lines #65

Conversation

SmaranTum commented Aug 24, 2022 • edited by burgholzer Loading

codecov bot commented Aug 24, 2022 • edited Loading

Codecov Report

burgholzer left a comment

Choose a reason for hiding this comment

burgholzer left a comment

Choose a reason for hiding this comment

burgholzer left a comment

Choose a reason for hiding this comment

SmaranTum commented Sep 5, 2022

burgholzer commented Sep 5, 2022

SmaranTum commented Sep 5, 2022

burgholzer commented Sep 5, 2022

burgholzer left a comment

Choose a reason for hiding this comment

SmaranTum commented Sep 5, 2022

burgholzer commented Sep 5, 2022

SmaranTum commented Aug 24, 2022 •

edited by burgholzer

Loading

codecov bot commented Aug 24, 2022 •

edited

Loading