Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor comparison methods of GenEllipsid, ellipsoid and hyperplane via inheriting these classes from modgen.common.obj.HandleObjectCloner #44

Open
pgagarinov opened this issue Dec 3, 2015 · 1 comment

Comments

@pgagarinov
Copy link
Member

Deadline: end of the semester

GenEllipsoid, ellipsoid and hyperplane classes are all inherited from elltool.core.AGenEllipsoid class that implements isEqualInternal method
that is designed for an element-wise comparison of two object arrays. HandleObjectCloner in its turn takes this even futher by introducing
separate methods for element-wise comparison, pair-wise comparison of multiple arrays and even supporting gt,le,lt,ge and sort operators based on
lexicographical comparison of BLOB sequences. The goal of this task is to get rid of isEqualInternal method by replacing it with the methods
inherited from HandleObjectCloner class. Doing so will cause the following changes in the way some of the comparison methods work:

I) isEqual method implemented in AGenEllipsoid now returns a logical array isEqualArr that contains a result of element-wise comparison of corresponding
object elements while isEqual implemented in HandleObjectCloner returns a scalar logical output that contains a result of comparison of multiple inputs (type
edit modgen.common.obj.HandleObjectCloner.isEqual to see for yourself). However HandleObjectCloner.isEqualElem does the same as AGenEllipsoid.isEqual. This means
that after inheriting AGenEllipsoid from HandleObjectCloner all tests (and other code) that used AGenEllipsoid.isEqual to do element-wise comparison will have to be
changed to use isEqualElem. The good thing however is that most of the code while calling isEqual did it solely for scalar objects which means this code expected
an output of isEqual to be scalar. Thus most of the code won't have to be changed.

II) AGenEllipsoid uses Matlab built-in implementation of isequal, isequaln and isequalwithequalnans which is based on comparing objects as binary sequences. HandleObjectCloner
however changes that by redirecting calls to these 3 methods to either isEqualScalarInternal or isEqualScalarAsBLOBInternal depending on what HandleObjectCloner.getComparisonMode
method returns (if asBLOB property is not specified). There is asHandle flag that can also be specified, if true then calls to isequal, isequaln and isequalwithequalnans are redirected to eq with asHandle=true flag and objects are compared as handles.

III) HandleObjectCloner also re-defines >=, <=,>,< == and ~= operators (same as ge, le,gt, lt,eq,ne). Since ellipsoid overrides <=,>=,<,> based on its own ellipsoid-based logic an implementation of this operators in ellipsoid class should not be touched. However, ne operator in ellipsoid class needs to be removed since HandleObjectCloner's implementation of the same operator is more correct.

IV) Some tests for ellipsoid, hyperplane and GenEllipsid classes use eq,==, isequal, isequaln and isequalwithequalnans methods to compare ellipsoids. There can be two reasons for that
a) a need to compare objects as binary sequences (this is the way the built-in implementation of isequal* methods work) i.e. with absolute precision using all content of the object including the fields like "absTol" that ordinary "isEqual" method does not compare.
b) Just a human error - a developer could have used isEqual but used isequal instead just because it looked more familiar.
These two case need to be treated separately - in case a) you need to replace calls with isEqual with "asBlob" flag while in b) you need to use just "isEqual" without asBlob.

The complete functionality of HandleObjectCloner can be easily studied by looking at the tests of this class in +modgen+common+obj+test+mlunit\HandleObjectClonerTC.m
test case.

Here is what you need to do step by step

  1. Remove implementation of isEqualInternal method in AGenEllipsoid and add inheritance from modgen.common.obj.HandleObjectCloner.
  2. Go through all tests for ellipsoid, GenEllipsid and hyperplane classes in elltool.core.test. package and make a method replacement based on items I)-IV) and Make sure all
    tests from elltool.core.test package pass.
  3. Make sure that all ET tests pass.
  4. Implement additional tests in a separate test case class in elltool.core.test.mlunit package for isEqualElem, isequal, isequaln,isequalwithequalnans, ne and eq methods.
    These tests should be made common for ellipsoid, hyperplane and GenEllipsid i.e. parametrized by these objects factory (i.e. one test case class should test all 3 classes).
    When doing so please refer to elltool+core+test+mlunit\HyperplaneDispStructTC and elltool+core+test+mlunit\EllipsoidDispStructTC test cases as example (both these test cases
    are inherited from elltool.core.test.mlunit.ADispStructTC class that contain the tests while sub-classes define certain methods that contain all the differences between
    of hyperplanes and ellipsoids).
@pgagarinov
Copy link
Member Author

You probably will find it useful to talk to Kirill about HandleObjectCloner - he also has an experience of using this class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant