UniversalGRAMM / UGRAMM

Universal GRAph Minor Mapper (UGRAMM)
https://universalgramm.github.io/
Other
2 stars 0 forks source link

Reinstanted the Constant in GRAMM #7

Closed HamasWaqar closed 2 months ago

HamasWaqar commented 2 months ago

Hey @ombhilare999,

I have reinstated Constant in GRAMM. The branch that currently has the change is addingConstant. Feel free to modify it to add the constant for the visualizer.

Fyi, I am going to spawn a new branch out of the addingConstant branch so I can add the pins in parallel.

Hamas

ombhilare999 commented 2 months ago

Let me access the addingConstant branch and make the visualize related changes.

HamasWaqar commented 2 months ago

Let me know when the changes are added

HamasWaqar commented 2 months ago

Hey @ombhilare999, A quick question, why did you move the vectors inPin and outPin from GRAMM.h to GRAMM.cpp? From my understanding, these vectors would be better suited in GRAMM.h.

ombhilare999 commented 2 months ago

Hi @HamasWaqar ,

When variables are defined inside the header files, it is considered as a bad practice. Since we call GRAMM.h in multiple cpp files then it is troublesome and gives error (you can try this by removing the changes with trying the compilation with the current Makefile and the setup). The variables should only be declared in the common header files and then should be defined in a single cpp where they will be used.

You can refer to this link: https://stackoverflow.com/questions/1164167/variable-declaration-in-a-header-file

ombhilare999 commented 2 months ago

Hi @HamasWaqar ,

I've pushed some quick fixes related to the constant for now (with the aim being no stalling on your side due to me). These will be the only changes for the visualizer concerning constant-related work at this stage. As described in the commit message, I've added support exclusively for the unpositioned graph. Before proceeding with adding constant support to the positioned graph, I would like to discuss a few things.

Here's a demo of the constant implementation:

image image

Also, I observed constant mapping failing for the FFT benchmark, please check it once:

HamasWaqar commented 2 months ago

I see. I was taught that the global variable used in the particular files must be stated in the H files in C++. This is more true or OOP in C++. This can be discussed later on, but I'll work with your code right now.

HamasWaqar commented 2 months ago

Thanks, I'll use these to add the pins. @ombhilare999 I think, could we wait to push the current changes for this branch to master until I have integrated the pins logic. This will help me not be stalled in my work. Thanks

ombhilare999 commented 2 months ago

Closing this, refer #9 for the details.