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.

Sign up for GitHub

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

Jump to bottom

adding tests for priors #519

Closed
wants to merge 2 commits into from
Closed

adding tests for priors #519

wants to merge 2 commits into from

Conversation

KrisThielemans
Copy link
Collaborator

currently failing...

@KrisThielemans
Copy link
Collaborator Author

I currently see the following failures:

  • Quadratic prior has edge effects, but aside from that a nearly constant diff between the analytical and numerical gradient
  • PLS prior is ok in the middie, but has strong edge effects.

@robbietuk could you try the QuadraticPrior with your Python test?

@robbietuk
Copy link
Collaborator

My python QP test seems reasonable. I do not have edge effects.

@KrisThielemans
Copy link
Collaborator Author

I've brought this up-to-date, but not changed the tests themselves.

@robbietuk
Copy link
Collaborator

Is it worth rebasing instead? The commit history and file changes are a bit of a mess :/

@KrisThielemans
Copy link
Collaborator Author

hmmm. weird. I'll do that.

@KrisThielemans
adding tests for priors
bb6b0df 
currently failing...
@KrisThielemans KrisThielemans changed the base branch from release_4 to master September 11, 2020 08:39
@KrisThielemans
Copy link
Collaborator Author

ok, it was on release_4, with some stuff happening in between. cleaner now

robbietuk
robbietuk reviewed Sep 18, 2020
View reviewed changes
src/recon_test/test_priors.cxx Outdated
target_type::full_iterator target_iter=target.begin_all();
target_type::full_iterator gradient_iter=gradient_sptr->begin_all();
target_type::full_iterator gradient_2_iter=gradient_2_sptr->begin_all();
const float eps = 1e-2F;
Copy link
Collaborator

@robbietuk robbietuk Sep 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is too large a perturbation that results in the test failing for QP. When this value is reduced to 1e-3 or 1e-4, the QP test passes.

@robbietuk
Copy link
Collaborator

Would resolve #153

@KrisThielemans
Update test_priors.cxx
850acf8 
reduced value of epsilon
@robbietuk
Copy link
Collaborator

Hi, I have made some changes to this PR, like including the relative difference prior (which fails) but I do not have permission to push to the branch.

@KrisThielemans
Copy link
Collaborator Author

I notice that Travis is green now. thanks for the advice on eps.

either do a PR on this one, or just start a new one (might be easiest). depending on what your changes are, we could merge this one first. As you prefer.

could you check the Codacy warning? https://app.codacy.com/manual/KrisThielemans/STIR/pullRequest?prid=5421028

@robbietuk
Copy link
Collaborator

robbietuk commented Sep 23, 2020 •
edited
Loading

I'll try merging to your branch and check Codacy. Also while travis is passing, I am not sure it is actually running test_priors as I get an error with PLS on my machine.

e: Codacy is upset because it thinks that target_iter is reassigned before use. This isn't true as we are updating the target using target_iter and computing the objective function value before target_iter is reassigned. Not sure how to make codacy happy as the code is fine.

@robbietuk robbietuk mentioned this pull request Sep 24, 2020
Merged
3 tasks
@KrisThielemans
Copy link
Collaborator Author

superseded by #695

@KrisThielemans KrisThielemans deleted the add_test_priors branch October 6, 2020 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robbietuk robbietuk robbietuk left review comments

Assignees
No one assigned
Labels
in-progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@KrisThielemans @robbietuk