SNUCSE-CTA / GI

Apache License 2.0
13 stars 12 forks source link

Regarding `checkSimpleInvarianats` #12

Closed NamYehyun closed 4 years ago

NamYehyun commented 4 years ago

checkSimpleInvariants defined in https://github.com/SNUCSE-CTA/GI/blob/5e0bfc82fd51c6f34ae075b6fdbf28558c477fbb/src/algorithm.cpp#L38

and colorByDegreeAndLabel defined in https://github.com/SNUCSE-CTA/GI/blob/5e0bfc82fd51c6f34ae075b6fdbf28558c477fbb/src/refine.cpp#L146

are doing the same thing, except that checkSimpleInvariants returns a boolean value and discards its product, while colorByDegreeAndLabel stores its product for later use and returns nothing.

I think there are two ways to handle this redundancy.

  1. Save the product of checkSimpleInvariants somewhere (probably in some new global variable) and use it in colorByDegreeAndLabel.
  2. Remove checkSimpleInvariants and let colorByDegreeAndLabel do all the jobs.

Approach 2 surely has a drawback; when non-isomorphic graphs are given as an input, unlike the current implementation, we have to go through additional costs of instantiating a new Coloring and initializing workspace memory. https://github.com/SNUCSE-CTA/GI/blob/0ae801608b48889ba975b903eb5c94c9284da874/src/refine.cpp#L30-L36 However, I think approach 1 is an ugly way to handle the problem.

I'd like to know what you think about this problem.

gmgu commented 4 years ago

The second approach looks fine with me.

NamYehyun commented 4 years ago

Handled as the second approach (see 903b91ea6aba5bda99f1992d564570d284bcfb09).