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

Get SCI macros tested + working #1

Merged
merged 8 commits into from Jan 7, 2021
Merged

Conversation

sritchie
Copy link

@sritchie sritchie commented Jan 6, 2021

This PR:

  • adds tests for some of the forms that you had already tested, @mk
  • massages the macros into shape, and adds tests for these too
  • adds a bunch more namespaces to the default SCI context

I also bound sicmutils.env as itself AND as user, instead of replacing it.

@mk , I'll add more comments inline!

Copy link
Author

@sritchie sritchie left a comment

Choose a reason for hiding this comment

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

Some comments giving "context" (yuk yuk, SCI joke) about the PR>

@@ -166,22 +166,26 @@
:else
(u/illegal (str "WTF range" range)))))

(defn binding-pairs [litfns]
Copy link
Author

Choose a reason for hiding this comment

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

I pulled the logic that lived outside of quasiquote land so that we had a bit less to duplicate in the sci macros namespace. I tested that this works, and the existing tests will exercise it too.

@@ -34,9 +34,11 @@
#(m/point->coords coordinate-system %)))
(s/structure->access-chains prototype))))

(defn ^:private quotify-coordinate-prototype
Copy link
Author

Choose a reason for hiding this comment

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

Rather than use the #' var resolution trick to get past private, let's just make this public.

(This has to be available to SCI because the macroexpanded body of let-coordinates refers to this symbol.)

@@ -48,6 +48,7 @@
[sicmutils.numerical.quadrature]
[sicmutils.mechanics.lagrange]
[sicmutils.mechanics.hamilton]
[sicmutils.mechanics.rigid]
Copy link
Author

Choose a reason for hiding this comment

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

this is a very cool namespace behind Colin's latest "tumbling T handle" demo that I want to recreate. We don't explicitly bind any of its symbols in env but I want it available for requiring.

@@ -340,7 +341,11 @@
zero-like one-like identity-like
numerical? freeze kind kind-predicate])

;; Macros. These work with Potemkin's import, but not with the Clojure version.
;; Macros. These work with Potemkin's import, but not with the Clojure
Copy link
Author

Choose a reason for hiding this comment

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

note for myself, let's leave it in!


(defn ->sci-var [[var-name the-var]]
[var-name (cond-> (deref the-var)
(-> the-var meta :macro)
Copy link
Author

Choose a reason for hiding this comment

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

I think this was from an earlier experiment, where we changed the meta, before we realized that we had to rewrite the whole macro. We actually want to filter all macros out.

(comment
(defn eval [form]
(sci/eval-string* ctx (pr-str form)))
- any pair removed whose value is a macro (tagged with `:macro true` metadata)
Copy link
Author

Choose a reason for hiding this comment

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

I think this does everything we want all bundled up.

compilation and interesting enough in their own right to expose to a user by
default."}
namespaces
#{'sicmutils.env
Copy link
Author

Choose a reason for hiding this comment

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

what a list!


(eval '(literal-function 'U))
Copy link
Author

Choose a reason for hiding this comment

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

I moved these to the tests.

@@ -31,8 +59,8 @@
`(let [[~@c-systems :as c-systems#]
(mapv m/with-coordinate-prototype
~c-systems
~(mapv #(sicmutils.calculus.coordinate/quotify-coordinate-prototype identity %) prototypes))
c-fns# (map coordinate-functions c-systems#)
~(mapv #(cc/quotify-coordinate-prototype identity %) prototypes))
Copy link
Author

Choose a reason for hiding this comment

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

woohoo, these all work, and are locked in to the tests!

@@ -102,7 +102,7 @@
(is (= "x^y" (s->infix (expt 'x 'y)))))

(deftest more-with-D
(af/with-literal-functions [f g [p [] 0]]
(af/with-literal-functions [f g]
Copy link
Author

Choose a reason for hiding this comment

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

I found this during a head-scratching moment trying to decode the syntax for the three arg version (it's name, domain, range of fn), and realized... I don't think an abstract function of NO arity can do anything...

whatever the answer is p is not used in the body here, so we can delete.

@@ -83,15 +83,17 @@
v (/ (- w b) M)]
(- (* w v) (F v))))))

(def Legendre-transform (make-operator Legendre-transform-fn "Legendre-transform"))
Copy link
Author

Choose a reason for hiding this comment

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

A little catch I saw when investigating the env namespace! The operator name should be a symbol, not a string.

Copy link
Owner

@mk mk left a comment

Choose a reason for hiding this comment

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

Looks great, just a minor remark about exposing the ns-map for sci. But I can also look into this tomorrow.

(let [ns-map (into {}
(map (fn [[k v]] [k (sci-ns v)]))
ns-map)
with-macros (merge-with merge ns-map macros/ns-bindings)]
Copy link
Owner

Choose a reason for hiding this comment

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

For nextjournal (and other downstream consumers) it might be good to also expose the plain sci namespace map (what with-macros is here). Without any assumptions of what the user ns is. Maybe this is what the ns-map var could be turned into?

Copy link
Author

Choose a reason for hiding this comment

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

yes, good call, that's much better. I made that fix and moved the 'user change into the nextjournal test PR.

@mk mk merged commit d1be1d1 into mk:sci-env Jan 7, 2021
@sritchie sritchie deleted the sritchie/sci_tests branch January 7, 2021 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants