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

MAGICL extensions refactor #125

Merged
merged 46 commits into from
Mar 23, 2021
Merged

Conversation

stylewarning
Copy link
Member

@stylewarning stylewarning commented Jan 30, 2021

This is a PR for #124 .

@stylewarning stylewarning marked this pull request as ready for review January 30, 2021 02:45
@stylewarning stylewarning changed the title WIP: MAGICL extensions refactor MAGICL extensions refactor Jan 30, 2021
@stylewarning
Copy link
Member Author

CC @kilimanjaro

This seems to be complete in the refactoring sense. This will probably require changes to be made downstream to quilc/qvm/etc.

@stylewarning
Copy link
Member Author

stylewarning commented Jan 30, 2021

If this PR merges, we could work on this issue honestly, including moving quilc's Lisp implementation of CSD. A lot of the matrix functions in quilc are here.

@stylewarning
Copy link
Member Author

The only thing I'm somewhat unhappy with is that this PR doesn't decide once and for all exactly how functionality is extended. Currently, it adds methods to a generic function. But we'll quickly have to ponder what happens when there are several competing methods implementing the same thing.

@@ -3,12 +3,15 @@

_Matrix Algebra proGrams In Common Lisp_ by [Rigetti Computing](http://www.rigetti.com). (née FLAIL: _Finally, Linear Algebra In Lisp!_)

(**Note**: The high-level interface is experimental and subject to rapid change.)
(**Note**: The high-level interface is experimental and subject to change.)
Copy link
Contributor

Choose a reason for hiding this comment

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

🐌

@stylewarning
Copy link
Member Author

One possible way of extending functions would be through a custom dispatch mechanism:

(defvar *profile* '(:lisp))

(defconstant +dispatch-functions+ 'dispatch-functions)

;;; Scaffolding

(defmacro define-dispatched-function (name args)
  (let ((profile             (gensym "PROFILE"))
        (available-functions (gensym "AVAIL"))
        (func                (gensym "FUNC")))
    `(defun ,name ,args
       (let ((,available-functions (getf (symbol-plist ',name)
                                         +dispatch-functions+)))
         (dolist (,profile *profile*)
           (let ((,func (getf ,available-functions ,profile)))
             (unless (null ,func)
               (return-from ,name (funcall ,func ,@args))))))
       (error ,(format nil "~S is not implemented in profiles ~~{~~A~~^, ~~}." name)
              *profile*))))

(defmacro define-dispatched-method (name profile funcallable-object)
  `(progn
     (setf (getf (getf (symbol-plist ',name) +dispatch-functions+) ',profile) ,funcallable-object)
     nil))

;;; Interface

(define-dispatched-function hypot (x y))
(define-dispatched-function erf (x))


;;; Lisp implementations

(defun lisp-hypot (x y)
  (format t "~&Hello from Lisp.~%")
  (sqrt (+ (expt x 2) (expt y 2))))

(define-dispatched-method  hypot :lisp 'lisp-hypot)


;;; C implementations

(declaim (inline %c-hypot))
(sb-alien:define-alien-routine ("hypot" %c-hypot) sb-alien:double-float
  (x sb-alien:double-float)
  (y sb-alien:double-float))

(defun c-hypot (x y)
  (declare (type double-float x y))
  (format t "~&Hello from C.~%")
  (%c-hypot x y))

(define-dispatched-method hypot :c 'c-hypot)


(declaim (inline %c-erf))
(sb-alien:define-alien-routine ("erf" %c-erf) sb-alien:double-float
  (x sb-alien:double-float))

(defun c-erf (x)
  (declare (type double-float x))
  (format t "~&Hello from C.~%")
  (%c-erf x))

(define-dispatched-method erf :c 'c-erf)



;;; Example

;; CL-USER> *profile*
;; (:LISP)

;; CL-USER> (hypot 3.0d0 4.0d0)
;; Hello from Lisp.
;; 5.0d0

;; CL-USER> (ignore-errors (erf 1.0d0))
;; NIL
;; #<SIMPLE-ERROR "ERF is not implemented in profiles ~{~A~^, ~}." {10087366B3}>

;; CL-USER> (let ((*profile* '(:lisp :c)))
;;            (erf 1.0d0))
;; Hello from C.
;; 0.8427007929497148d0

;; CL-USER> (let ((*profile* '(:lisp :c)))
;;            (list (hypot 3.0d0 4.0d0) (erf 1.0d0)))
;; Hello from Lisp.
;; Hello from C.
;; (5.0d0 0.8427007929497148d0)

;; CL-USER> (let ((*profile* '(:c :lisp)))
;;            (list (hypot 3.0d0 4.0d0) (erf 1.0d0)))
;; Hello from C.
;; Hello from C.
;; (5.0d0 0.8427007929497148d0)

@braised-babbage
Copy link
Contributor

Re: this mechanism, how would this interact with the current dispatch on tensor type?

@stylewarning
Copy link
Member Author

Re: this mechanism, how would this interact with the current dispatch on tensor type?

The only thing I'd imagine is allowing generic functions to be called (which are just functions after all), and method-not-found errors can be caught so it can move on to the next set of functions in the policy.

@stylewarning
Copy link
Member Author

@notmgsk If we change this, are you prepared to make the downstream changes as necessary for quilc etc?

@stylewarning
Copy link
Member Author

stylewarning commented Mar 19, 2021

I went ahead and implemented the framework described in my Feb 3 comment, except I fleshed it out a lot more.

Check out how EXPM is implemented to see it in action.

There are now two TODOs:

  • Write documentation for how to extend functions with new backends.
  • Convert the LAPACK functions to the backend functionality
  • Write other existing purely Lisp-based functions into backend functions

Despite TODOs, I'd eagerly like to hear feedback from @notmgsk @kilimanjaro at least.

@stylewarning stylewarning marked this pull request as draft March 19, 2021 03:46
@stylewarning
Copy link
Member Author

@notmgsk I got rid of MAGICL/FANCY and stuck with MAGICL for loading all libraries. However, I introduced MAGICL/CORE for pure-Lisp functionality.

@stylewarning
Copy link
Member Author

@colescott All PR feedback should be addressed. Thanks for the feedback, very helpful.

Copy link
Member

@colescott colescott left a comment

Choose a reason for hiding this comment

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

lgtm :shipit:

@stylewarning
Copy link
Member Author

The remaining commits after @colescott's approval do three things:

  1. Small "inessential" clean-ups (like removing deprecated ASDF functions)
  2. Improving backwards compatibility with MAGICL users' code
  3. Making deprecation warnings stronger

Copy link
Contributor

@braised-babbage braised-babbage left a comment

Choose a reason for hiding this comment

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

I really like this. Adding a few comments/questions re: documentation.

doc/dev-how-to.md Outdated Show resolved Hide resolved
(asdf:test-system :magicl)
```

You currently need all of the extensions working for the tests to run.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth it to throw in a few easy tests that exercise magicl/core explicitly? More broadly, we should probably make an issue to test various backends explicitly.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought in a separate test we could write test functions which iterate over the backends using with-backends or binding *backends* directly. I don't think I want to do it in this PR though. Made issue #128

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/backend-function.lisp Show resolved Hide resolved
@stylewarning
Copy link
Member Author

@kilimanjaro I fixed all the issues you pointed out.

I also regenerated the LAPACK bindings from the latest sources... I know it's not appropriate for this PR technically, but it came as a result of fixing bugs in MAGICL-GEN that were uncovered in code review.

@notmgsk notmgsk merged commit 4b22bfa into quil-lang:master Mar 23, 2021
@stylewarning stylewarning deleted the feature/separate branch March 23, 2021 22:29
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.

4 participants