rcsb / symmetry

:ferris_wheel: Detect, analyze, and visualize protein symmetry
GNU Lesser General Public License v2.1
26 stars 16 forks source link

CeSymm runs are not independent #29

Closed sbliven closed 8 years ago

sbliven commented 10 years ago

Some information is leaked between runs of CeSymm. The can be demonstrated by running the following test (which should be included in CeSymmTest) with assertion enabled:

    public void testIndependence() throws IOException, StructureException {
        String name;

        // Perform alignment to determine axis
        Atom[] ca1, ca2;
        AFPChain alignment;
        RotationAxis axis;
        double[] coefs,expectedHarmonics;

        CeSymm ce = new CeSymm();

        name = "1MER.A";
        ca1 = StructureTools.getAtomCAArray(StructureTools.getStructure(name));
        ca2 = StructureTools.cloneCAArray(ca1);
        alignment = ce.align(ca1, ca2);

        //ce = new CeSymm();// work around bug

        name = "1ijq.A:377-641";
        ca1 = StructureTools.getAtomCAArray(StructureTools.getStructure(name));
        ca2 = StructureTools.cloneCAArray(ca1);
        alignment = ce.align(ca1, ca2);
    }

It causes an AssertionError in CeCPMain.java:165 in postProcessAlignment, indicating that either the matrix or the alignment lengths are being leaked from the first assignment. Creating a fresh CeSymm instance for each run fixes the error.

There is also a commented-out test in CeSymmTest which likely is related to this bug. CECP should also be checked to see if this bug applies.

andreasprlic commented 10 years ago

Does this also apply to jCE and jFatcat?

sbliven commented 10 years ago

I suspect it was introduced in the CECP code, so it should be fine in CE/FATCAT.

dmyersturnbull commented 10 years ago

It looks like this should be fine to close now, if I understand the entirety of the issue.

sbliven commented 10 years ago

Nice. I'd been looking in the wrong direction on that problem. That should do it!

I've merged your fork into master (d6612db) and I'm closing this.

sbliven commented 10 years ago

These tests are failing again, so I'm reopening this issue.

Possible duplicate of #37.

andreasprlic commented 10 years ago

for now a workaround is to always create a new CeSymm instance

CeSymm ce = new CeSymm();
lafita commented 8 years ago

I fixed this in a biojava commit c1b7951. I created the Test proposed by @sbliven and no error occurs. I tried to reduce as much as possible the member variables of CeSymm and reset them every "align" call, so that the same object with the same parameters can be used to analyze other structures.