Classification should either be static, or not static, not a mix

[stanleybak]

Classification.java is currently partially static and partially not static. setLinearMatrix refers to varId and linearMatrix. These variables seem like they should be local to the Classification object, not global singletons (which is what happens when you make them static). Also 'ha' has to be set before using Classification.

This makes the object hard to use, since it's not clear what needs to be setup before I can extract the A matrix (do I need to set the global ha? What about varId?). I think a clean interface would have a simple static method that takes in either just the AutomatonMode, or the flowdynamics map and a list of variables in the desired order.

[stanleybak]

Also, classification fails on the following test. The main problem is the minus 'x'. in the derivative of x. Entry 0,0 of the A matrix should be -1, but the method returns 0. Please fix.

@Test
@Test
    public void testClassifySubtract()
    {
        String[][]dy = {{"x","-x - 2 * y -0.2 * u"}, {"y", "4 * x - 3 * y + 2 * u"}, {"u", "1"}};
        // Automaton with three variables ['x', 'y', 'u'] and flows: 
        // x' == -x - 2 * y -0.2 * u
        // y' == 4 * x - 3 * y + 2 * u
        // u' == 1

        Configuration c = AutomatonUtil.makeDebugConfiguration(dy);
        BaseComponent ha = ((BaseComponent)c.root);
        AutomatonMode mode = ha.modes.values().iterator().next();

        Classification cls = new Classification();
        Classification.ha = ha;
        cls.setVarID(ha); 
        cls.setLinearMatrix(mode);

        double TOL = 1e-9;
        Assert.assertEquals(-1, Classification.linearMatrix[0][0], TOL);
    }

[stanleybak]

Classification.java also needs cleanup. There are formatting issues, some of the 'tabs' are 4 spaces, some are 6. Please reformat. the findCoefficient() method has some lines that are over 150 characters, which should be split up into simpler sub-statements to make it more readable.

[ttj]

Luan: please resolve this bug and the issues Stan raised as you're most familiar since you wrote the classifier.

Also, Luan: Please try to use an IDE or development style that is consistent, e.g., I'm just using Eclipse and just call ctrl+shift+F on occassion to reformat and fix the code formatting to ensure it's consistent: http://stackoverflow.com/questions/15655126/how-to-auto-format-code-in-eclipse

Presumably whatever you're using has a similar way to do this, so please make sure the code quality is good. Here are some general guidelines Stan prepared: https://github.com/verivital/hyst#code-quality

While we get busy and sometimes have to rush for deadlines, it's very important to keep the code quality as high as possible and clean things up once a deadline has passed if some things get hacked in to make the deadline.

[LuanVietNguyen]

Yes, I will work on it tonight. I'm using Netbeans but I think it's also has an option to automatically fix the code format.

[LuanVietNguyen]

I already resolved the bug, this bug is raised up in case the operator is negative, I pushed the fix on my folk.
Stan: please take a look on it and let me know if you want to merge my change immediately to keep the main branch being bug-free.

[stanleybak]

I've merged the bug fix into the main line; thanks for the quick response.

Could you also address the first comment here, where Classification is partially static and partially non-static? What is the reason for the Classification object to maintain state? I may be wrong, but I think a single static method to do the classification is all that's necessary.

[LuanVietNguyen]

Yes, I think you're right. I will change all methods in Classification class to be static.