-
Notifications
You must be signed in to change notification settings - Fork 114
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
Enable phil-scoped linear solver backend selection #162
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just some general comments
libtbx/auto_build/bootstrap.py
Outdated
anonymous = ['git', | ||
'git@github.com:eigenteam/eigen-git-mirror.git', | ||
'https://github.com/eigenteam/eigen-git-mirror.git', | ||
'https://github.com/eigenteam/eigen-git-mirror/archive/master.zip'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to get the bleeding-edge development sources, or rather a stable version? If the latter I would suggest
anonymous = ['git', '-b branches/3.3'
'git@github.com:eigenteam/eigen-git-mirror.git',
'https://github.com/eigenteam/eigen-git-mirror.git',
'https://github.com/eigenteam/eigen-git-mirror/archive/branches/3.3.zip']
scitbx/lstbx/normal_eqns.py
Outdated
try: | ||
self.step_equations().solve(args[1]) | ||
except: | ||
self.step_equations().solve() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will not only run solve() if the function is run without arguments, but also if any uncaught exception is encountered during solve with argument or if the user presses CTRL+C. I don't think that is what you want.
Also not quite sure why you only use the second argument - what happens with the first? Some documentation as to what arguments you expect here may be helpful.
scitbx/lstbx/normal_eqns_solving.py
Outdated
@@ -122,6 +122,20 @@ def __init__(self, non_linear_ls, **kwds): | |||
self.non_linear_ls = journaled_non_linear_ls(non_linear_ls, self, | |||
self.track_gradient, | |||
self.track_step) | |||
try: | |||
linsolver_param = non_linear_ls.linsolver | |||
except: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again this catches Ctrl+C. Please use specific exceptions.
In this case I think you can get rid of the entire try-except block and just use
linsolver_param = getattr(non_linear_ls, 'linsolver', None)
scitbx/lstbx/normal_eqns_solving.py
Outdated
elif linsolver_param == 'bicgstab': | ||
self.linsolver = lsb.bicgstab | ||
else: | ||
self.linsolver = lsb.ldlt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternative:
self.linsolver = {
'ldlt': lsb.ldlt,
'llt': lsb.llt,
'cg': lsb.cg,
'bicgstab': lsb.bicgstab,
}.get(linsolver_param, lsb.ldlt)
or even
self.linsolver = getattr(lsb, linsolver_param or 'ldlt', lsb.ldlt)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cheers Markus, these changes all seem like good ideas. I'll hopefully get time to add them in the coming week or so.
I've removed the previous try-except structures, and moved the phil param to affect only the Eigen sparse enabled code. Eigen has been set to 3.3 release branch. This may benefit from master at some time in future as certain algorithms are getting OpenMP support, but stable is fine currently. |
The Eigen sparse linear solver backend of the LevenbergMarquardt algorithm can be chosen using the phil parameter
levmar.linsolver=*ldlt llt cg bicgstab
ldlt
= Direct Cholesky LDLTllt
= Direct Cholesky LLTcg
= Iterative conjugate gradientbicgstab
= Iterative biconjugate gradient stabilised