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

Refactor log-determinant estimation code to work without computation engines #4074

Closed
karlnapf opened this issue Jan 8, 2018 · 7 comments
Closed

Comments

@karlnapf
Copy link
Member

karlnapf commented Jan 8, 2018

We want to delete the files in lib/computation/*

However, we don't want to delete @lambday 's nice code to estimate log-determinants of large sparse matrices.

This task is to re-factor things such that we can delete the computation classes. This is a nice entrance task that does only require to read/refactor existing code and tests. This will give you a nice first exposure to the internals of shogun without being overwhelming. Good non-trivial entrance task therefore.

Here is a rough list of steps
0. Read through the code, coming from the log-determinant examples

  1. Compile Shogun withe the log-det code enabled (see guards in code).
    2, Isolate and run all log-det tests
  2. Delete all files in shogun/lib/computation. Re-run cmake and make. The build is now broken, i.e. shogun won't compile
  3. Fix the build (will require slight API changes)
  4. Make sure all tests work
  5. Send PR
@shubham808
Copy link
Contributor

@karlnapf i want to take this up.

@shubham808
Copy link
Contributor

After looking at the code I got rid of computation engines and directly computed jobs treating them as independent jobs.
So I deleted engines directory and then debugged to get everything to work.
Is this the right approach ? And should I keep going similarly to eventually get rid of all classes in computation directory ?

@karlnapf
Copy link
Member Author

yes, thats exactly it. make sure to send a pr soon to we can give feedback.
Keep in mind we also want to get rid of independent jobs.
Best would be if you used openmp instead for now

@shubham808
Copy link
Contributor

shubham808 commented Jan 16, 2018

@karlnapf using omp is a good idea. I got rid of IndependentJob. However, I am stuck at getting rid of JobResult classes also i think keeping them around would simplify things.

@salonirk11
Copy link
Contributor

Also, is it necessary to remove lib/computation/aggregator ? Can we reuse these classes from mathematics/linalg/rattaprox/logdet/computation/aggregator?

@karlnapf
Copy link
Member Author

Everything in the computation folder should go

This was referenced Jan 19, 2018
@karlnapf
Copy link
Member Author

karlnapf commented Apr 4, 2018

Done via #4103

@karlnapf karlnapf closed this as completed Apr 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants