-
Notifications
You must be signed in to change notification settings - Fork 7
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
Comparison of ellipsoidal objects is done only based on relative precision, absolute precision is ignored #14
Comments
Hellow. I right understand, that I should write tests fo protected method elltool.core.AGenEllipsoid.isEqualInternal? Shoud I create new class heir in my test for access this metod? |
No, isEqual calls isEqualinternal so you should write tests for isEqual. Just study the source code of ellipsoid.isEqual and everything will become clear. |
Good evening, I'm analyzed elltool.core.AGenEllipsoid.isEqualInternal, and understanded, that in this function variable tollerance = ell1Obj.RelTol, and than it pass in function absrelcompare like AbsTol, if I write test like this |
Also i think it's better in function elltool.core.AGenEllipsoid.isEqualInternal selected absTol=min(ell1Obj.absTol,ell2Obj.absTol) and relTol = min(ell1Obj.relTol, ell2Obj.relTol). Because now the elementary property of symmetric (isEqual(ell1Obj,ell2Obj))=isEqual(ell2Obj,ell1Obj). dosen't. work The function work so wrong (in my opinion) that i can write a lot of fail tests but its won't be real usefull. |
ell1=ellipsoid(..., precision 1) or like this ell1Vec=ellipsoid.fromRepMat( precision 1)
Q2(1,1)=Q2(1,1)+1e-4; doesn't check for symmetry of isEqual so you cannot skip the symmetry test, it should be there as well. Please think of other cases of incorrect behavior of isEqual and see if they all are covered by your tests. If not - you need to write more tests before fixing isEqual. |
Hellow, i have question about how should works isEqual an ideal, if we have two Vecs of Ells like this or we should use as absTol1 the min_{i=1,2}(absTol(i)1) and relTol1 is the min_{i=1,2}(relTol(i)1) i.e. we have in common case we should use diffrent absTol and relTol for coporation the first pair and the second |
absTol and relTol should be calculated for each pair separately i.e. abs1Tol=min(abs11Tol,abs21Tol) - absolute tolerance for comparing Ell1Vec(1) and Ell2Vec(1) The same rule should be used for relative tolerance. Thus each pair is compared independently of other pairs. Just in case - Ell1Vec is a bad name - see our naming convention. |
Hello, I decided restart my work. I did clone last master version and when i tried install toolbox there was one problem... adding Warning: files
Error in modgen.io.isdir (line 16) Error in modgen.system.ExistanceChecker.isDir (line 38) Error in modgen.containers.ondisk.AHashMap (line 97) Error in modgen.containers.ondisk.HashMapXMLMetaData (line 35) Error in modgen.configuration.ConfRepoManager (line 86) Error in modgen.configuration.AdaptiveConfRepoManager (line 59) Error in elltool.conf.ConfRepoMgr (line 17) Error in elltool.conf.Properties.init (line 47) Error in ellipsoidsinit (line 7) Error in s_install (line 11) is it important? |
I'm restarted matlab. And all things are OK. Sorry, for last comment |
This is a normal behavior - see installation instructions in Wiki. The reason you need to restart Matlab is to add jar (Java archives) to a static java path of Matlab. |
Hello. I already asked this question, but I want to clarify. Should I change stucture of function and compare each pairs Or should I select union absTol and relTol and compare arrays? Now i write tests and want understand how isEqual should work. |
Yes, you need to change isEqualInternal a little bit - you should use the first output arguments of getAbsTol and getRelTol methods i.e. arrays of absTol and relTol instead of second outputs that are scalar values of these properties (see help headers of getAbsTol and getRelTol for more details). This way you will be able to use the tolerances as I described above in my comment on Nov 12. |
Hello, I updated to actual master. And, if I riqht understand, I shoud correct function isEqualInternal now. I have an question about an function. function SComp=formCompStruct(SEll,SFieldNiceNames,... If I correct understand, now in function isEqualInternal it use for rename struct fields to NiceNames,
|
Please have a look at #42 (comment) - I expect Alexander to fix this problem tomorrow. Anyways,
|
Last comment, I asked about method formCompStruct(), but I looked formCompStruct() in class GenEllipsoid, and in real formCompStruct() from class ellipsoid work. Sorry for my mistake. Nonetheless I have some questions about formCompStruct() from class ellipsoid. methods (Static)
Can I deleted commands In real it doesn't matter how the structure's field are named for their comparations, becase in structcomparevec() the command fieldnames() is using. |
I of course can try to write adequat test for full code, that check diffence between absTol and relTol, but extract the matrix root from two ShapeMatrixs before compare them..., can I delete this command? |
Can you please name all your questions with sequential numbers, not always 1,2 in each comment. Otherwise it is hard to refer to specific points in your comments. Anyways, please have a look at #42 (comment)
Does it make sense? |
Yes, thank you. |
I added some fail tests
+--------------------------------------------------------------------------------+ And I hope they are right. However the question of creartion some test for repMat method rest, because I don't understand what test should I do? Of corse all test that right for two ells, have analogues for repMat array, but I don't want copypaste. And more logical test, in my opinion, is cheking that: if smth right for ell1 and ell2, then the same right for repMat(ell1), repMat(ell2), but these test don't fail now. Should I do any another test? |
Sorry, I forgot add issue number in my commit message. Is this very bad? |
I will review your changes tomorrow, yes, it is not a good thing - if you can - please revert changes made in your branch and make a new commit with a corrected commit message, this is easy to do. |
I reverted changes and made new commit, but I'm not sure, that I did all right. |
Please revert your commit and make it again with a correct commit message this time.
if such asset fails a content of reportStr is displayed making it possible to understand why the test failed.
mlunitext.assert(all(aMat==bMat)); because all((aMat==bMat) returns a logical vector while assert expects a scalar. You should use all(aMat(:)==bMat(:))
|
And I wrote one test that chek this situation. Also there is an test thet chek situation when ellipsoids aren't equal if we use only absTol but should be equal when we use two presision parametrs. The case when relTol is really important. Can you clarify what another test should I add? |
|
When I asked about copy past I mean logical copypast, i.e. |
Another example of copy-pasting is
the only difference between these 3 lines are values of absTol and relTol. If create a nested then the block above will be transformed into testEll1=create(1e-4,1e-4); which is much more compact but not compact enough. Thus the text step would be creating one more nested function which will accept a list of absTol,relTol pairs and produce a vector of ellipsoid objects. I.e. testEll1, ... , testEll6 should be returned by this function as testEllVec.
|
I corrected the function. But the code is still unstabel. There is 6 failures, but I don't know what was failed,because error_fail_list.txt is empty? |
It is not empty, please be more attentive: https://groups.google.com/forum/#!topic/ellipsoids-tests-notification/ae6TiIeNmf8 |
I looked the test that was failed. I'm afraid that all of them had been written, such way, that all of them passed with wrong isEqual. Because in all test the relTol are used, as the shift parametr). (Expcept may be testEllunionEa, I dont't really understand what's happend in this test yet) |
Right. You need to figure out how these tests work and modify them so that they pass keeping in mind that now isEqual accounts for relTol as well. |
Ok
|
EllTbx_Win64_2015b » issue_14_IvanovaElena - Build # 12 - Fixed: I right understand, that all tests pass now? |
What should I do now? |
function tolVec=getTol(ellFirstArr,ellSecArr) return absTol and relTol separately instead of tolVec vector. All this is to make
classdef PrameterizedTC < mlunitext.test_case Also, in the same code should be outside of if else then because it will always work even when nargin(varargin)==0
num2cell is not necessary, jus use {} like this {1,2;3,4} Same goes for
insted of if nargin==2
cent,mat are incorrect names - where are suffices for matrices and vectors?
all you need to do is to replace replace the expected messages with the correct ones Same goes for all other places that contained checks for expected messages. You cannot |
I did 24)-32) |
You can merge to master now, please follow these steps:
While you wait for the test results you can start working on your next task: #44 |
Sorry, I have a probllem with merge? git.exe merge --no-ff remotes/origin/issue_14_IvanovaElena error: cannot stat 'products/+elltool/+core/@ABasicEllipsoid': Permission denied git did not exit cleanly (exit code 128) (375 ms @ 24.02.2016 23:42:55) |
I think this is because you didn't close matlab or some other app is blocking those files. Please reboot your pc just in case. Then switch/checkout to origin/master with checkbox "recreate branch if exisits". This will create a fresh copy of local master. Then switch to this local restored master and run merge again. |
You should always look at the latest build, no matter when you receive emails. You tests have passed. |
Hello! Sorry to bother you. I use Matlab 2018a. Please allow me to explain my installation process to you first. When I installed ET-2.1 in MATLAB 2018a, start_matlab2015b_win64 did not work; So I run s_install.m and the result is as follows:
|
Deadline - 28th of October, by that time all changes and tests should be merged into "master" branch.
Comparison of ellipsoid objects doesn't take into account an absolute precision, see elltool.core.AGenEllipsoid.isEqualInternal:
Line 31: [~, tolerance] = ellFirstArr.getRelTol;
Line 60-62:
This needs to be fixed by using absolute precision along with relative precision (modgen.struct.structcomparevec can actually accept both).
The first step should be writing the tests that "capture" this incorrect behavior. To do that study modgen.common.absrelcompare function and the way it works. The best ways to do that - study its code and the tests for the function (look at "testAbsRelCompare" test in modgen.common.test.mlunit.mlunit_test_common test case).
Once you understand the way absrelcompare works you can create tests by constructing ellipsoids with different precisions of calculation (this can be done via passing additional properties to "ellipsoid" constructor like this ellObj=ellipsoid(centVec,shapeMat,'absTol',1e-4,'relTol',1-3')) and calling isEqual to compare the constructed ellipsoid objects. The tests should be constructed in a such way that they would pass if the logic were correct.
The tests should be put into elltool.core.test.mlunit.EllipsoidTestCase. All the tests should fail.
After that you make the fix to isEqualInternal method and make sure that all the tests start to pass. If you change the tests to make them pass - roll back the change in isEqualInternal and make sure once again that they fail prior to the change. Thus the tests should all fail prior the change and should all pass after the change.
The text was updated successfully, but these errors were encountered: