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

Enable phil-scoped linear solver backend selection #162

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

mlxd
Copy link
Contributor

@mlxd mlxd commented Apr 4, 2018

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 LDLT
  • llt = Direct Cholesky LLT
  • cg = Iterative conjugate gradient
  • bicgstab = Iterative biconjugate gradient stabilised

@mlxd mlxd requested a review from nksauter April 4, 2018 23:13
Copy link
Member

@Anthchirp Anthchirp left a 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

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']
Copy link
Member

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']

try:
self.step_equations().solve(args[1])
except:
self.step_equations().solve()
Copy link
Member

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.

@@ -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:
Copy link
Member

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)

elif linsolver_param == 'bicgstab':
self.linsolver = lsb.bicgstab
else:
self.linsolver = lsb.ldlt
Copy link
Member

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)

Copy link
Contributor Author

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.

@mlxd
Copy link
Contributor Author

mlxd commented Apr 5, 2018

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.

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 this pull request may close these issues.

None yet

2 participants