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

adjoint tests sometimes fails for Gadgetron #637

Closed
KrisThielemans opened this issue Apr 25, 2020 · 15 comments · Fixed by #644
Closed

adjoint tests sometimes fails for Gadgetron #637

KrisThielemans opened this issue Apr 25, 2020 · 15 comments · Fixed by #644

Comments

@KrisThielemans
Copy link
Member

(at least) one job on linux is failing after #636 https://travis-ci.org/github/SyneRBI/SIRF/builds/679325885. Error is simply

FAIL: test2.test_main
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/.local/lib/python3.5/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/home/travis/build/SyneRBI/SIRF/src/xGadgetron/pGadgetron/tests/test2.py", line 70, in test_main
    raise AssertionError("Gadgetron operator is not adjoint")
AssertionError: Gadgetron operator is not adjoint

https://travis-ci.org/github/SyneRBI/SIRF/jobs/679325894#L31331

It'd be more useful to have some diagnostics as well then.

@KrisThielemans
Copy link
Member Author

likely a border case on the numerical threshold as there's nothing special for that job as far as I can see.

@KrisThielemans
Copy link
Member Author

It's only that one job https://travis-ci.org/github/SyneRBI/SIRF/builds/679325850

@KrisThielemans
Copy link
Member Author

@AnderBiguri I'll have a look later, unless you volunteer...

@AnderBiguri
Copy link
Contributor

@KrisThielemans im happy to have a look.

@AnderBiguri
Copy link
Contributor

AnderBiguri commented Apr 25, 2020

@KrisThielemans I cannot reproduce. Gadgetron adjoint test always return results 2 orders of magnitude lower ([3~50]e-7) than the threshold (10e-5). Is there something particular from that job that can have an influence?I can see its Xcode, which is Mac (I dont have access to Mac), but the rest of the flag combination seems OK with other builds? Its hard to know as they are not always clear in the flag list.

@KrisThielemans
Copy link
Member Author

@AnderBiguri the job listed in the first comment here is a linux job actually. It's more than likely that you cannot reproduce it as the other jobs are fine.

i don't see anything in the flags really. I think best thing to do is to write diagnostics to stdout when the test fails. Otherwise we're just in the dark.

@AnderBiguri
Copy link
Contributor

@KrisThielemans fair. The fucntion has print statements, however they do not work in the test. Other tests use sys.stderr.write() to display, but I did not want to put this inside the function. A possible alternative is making is_operator_adjoint return multiple outputs, which can be the errors and dot products. Open to other ideas.

@KrisThielemans
Copy link
Member Author

any reason we wouldn't have a test like this

if not is_operator_adjoint(am, verbose=true):
   ...

(or whatever appropriate Python syntax is)

@AnderBiguri
Copy link
Contributor

AnderBiguri commented Apr 25, 2020

@KrisThielemans yes, what I mentioned in my previous comment. print does not print on the ctests. I have only managed to print to console during ctest via sys.stderr.write()

In any case, we can always increase the error bound by doing is_operator_adjoint(am, max_err= 10e3):, for example, if we suspect that this is the problem.

@KrisThielemans
Copy link
Member Author

are you sure that print doesn't print? That is: ctest pipes everything to a log file. We run ctest --output-on-failure to get it to the Travis log as well, https://github.com/SyneRBI/SIRF-SuperBuild/blob/b1ea877a68b17daaebeae08e4c92a42f150d221b/.travis.yml#L491.

By the way, I think the verbose should default to True for your test, as the output is quite small (and helpful)

@AnderBiguri
Copy link
Contributor

Ah @KrisThielemans my bad. Still a lot of these things elude me. Defaulting to True should be a tiny change, yet I still do not know what to do to fix this issue.

@KrisThielemans
Copy link
Member Author

yet I still do not know what to do to fix this issue.

no sure. let's see the diagnostics first!

@KrisThielemans
Copy link
Member Author

funny. the relevant job now passed.

let's wait for the OSX tests but this update is good in any case.

@KrisThielemans
Copy link
Member Author

This seems to have disappeared, even though the only change in #638 was making verbose output the default. As there are no job failures anymore, I'll close this.

@KrisThielemans
Copy link
Member Author

KrisThielemans commented Apr 29, 2020

@AnderBiguri We have one with a failure

Testing AcquisitionModel: Iteration 1/25
Pass, with a with normalized error of 1.1071005495298965e-05 (max: 0.0001)
Testing AcquisitionModel: Iteration 2/25
Pass, with a with normalized error of 5.671866958080409e-07 (max: 0.0001)
Testing AcquisitionModel: Iteration 3/25
Pass, with a with normalized error of 4.4371515091084564e-05 (max: 0.0001)
Testing AcquisitionModel: Iteration 4/25
AcquisitionModel is not adjoint, with normalized error of 0.00011278952485286853 (max: 0.0001)

seems indeed that our threshold is just too optimistic. Change it to 0.0002?

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

Successfully merging a pull request may close this issue.

2 participants