Retire value protocol in favor of multimethods #149

littleredcomputer · 2023-10-10T17:52:49Z

Value is now renamed IKind since kind is now the only method that inhabits the protocol.

The other methods have been made into Emmy generics with defmulti.

Next step (after cljs level-up) is to rename the remaining one-method Value protocol to IKind.

…with cljs numeric tower, but this puts things back into semi-usable state for the night)

some of these are interesting... how were they working before?

codecov · 2023-10-10T18:07:48Z

Codecov Report

Attention: 93 lines in your changes are missing coverage. Please review.

Comparison is base (e16b569) 86.05% compared to head (49a92a3) 87.34%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #149      +/-   ##
==========================================
+ Coverage   86.05%   87.34%   +1.29%     
==========================================
  Files          98       98              
  Lines       15335    15383      +48     
  Branches      834      845      +11     
==========================================
+ Hits        13197    13437     +240     
+ Misses       1304     1101     -203     
- Partials      834      845      +11

Files	Coverage Δ
src/emmy/abstract/function.cljc	`84.72% <100.00%> (+0.80%)`	⬆️
src/emmy/calculus/covariant.cljc	`88.29% <100.00%> (+2.83%)`	⬆️
src/emmy/calculus/form_field.cljc	`89.83% <100.00%> (ø)`
src/emmy/calculus/map.cljc	`92.39% <100.00%> (ø)`
src/emmy/calculus/vector_field.cljc	`98.09% <100.00%> (ø)`
src/emmy/differential.cljc	`93.52% <100.00%> (+0.33%)`	⬆️
src/emmy/env.cljc	`94.80% <ø> (ø)`
src/emmy/expression.cljc	`74.81% <100.00%> (+1.73%)`	⬆️
src/emmy/expression/analyze.cljc	`94.16% <100.00%> (ø)`
src/emmy/expression/compile.cljc	`85.12% <100.00%> (ø)`
... and 37 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

CHANGELOG.md

sritchie

Still reviewing but here is my first batch of comments, @littleredcomputer !

sritchie · 2023-10-12T21:52:56Z

CHANGELOG.md

+      of the exponent; now it returns the one-like of the base, which is
+      correct.
+
+    - The generic `negative?` required Numbers instead of Comparables;


curious to see how this is done, as I introduced this bug, not knowing you could do it another way... do you compare now to the zero-like of the type?

Yup! I was inspired by similar examples you had designed.

package.json

patches/fraction.js+4.3.7.patch

sritchie · 2023-10-12T21:55:30Z

src/deps.cljs

@@ -1,4 +1,4 @@
 {:npm-deps
 {"complex.js" "^2.1.1"
-  "fraction.js" "^4.2.0"
+  "fraction.js" "^4.3.6"


I'm worried that your patch won't come along for the ride and derivative projects like emmy-viewers will see a failure. Would we fix this by staying on an older version? We could also pull bigfraction.js into our project with proper credit in the header... I haven't done this but there must be a way to pull JS out of resources in a way that doesn't affect downstream folks? hmmm

That's a good comment, and the developers of Fraction.js do suggest just "vendoring" the bigfraction.js if that's the API you want. I'm not sure quite where to put it.

On the other hand, the patches get swept in by the post install hook in package.json, so this feels like a principled approach...but if it does frustrate consumers of Emmy then it has to go.

I experimented using a local "vendored" version of bigfraction.js: doing that is easy enough but it needs a package.json to go with it. Fine, but that seems morally equivalent to the patch-package technique I've used. Since CLJS compilation can't happen without nom i, and that will take care of applying the patch, are you still worried?

My concern is about downstream libraries like Emmy-viewers - will all dependents now have to configure this patch as well? I would be fine pinning to an older version of Fraction as well to avoid this!

sritchie · 2023-10-12T22:03:56Z

src/emmy/abstract/function.cljc

@@ -230,9 +220,9 @@
          litfns)))

 (u/sci-macro with-literal-functions [litfns & body]


We definitely want this indentation to match defmacro... can you try adding

{:clj-kondo/lint-as 'clojure.core/defn}

to emmy.util/sci-macro to see if vscode picks it up?

Of course

{:clj-kondo/lint-as 'clojure.core/defmacro}

makes more sense but I think defn might be the only supported option...

fixing by adding a .cljfmt-vscode.edn file. (vs-code reads this file for indentation formatting, but the one that exists has :indentation false so it's not suitable for the editor. So the new one will rectify that as well as supplying the indentation rule needed here

sritchie · 2023-10-12T22:08:37Z

src/emmy/collection.cljc

@@ -235,33 +220,23 @@
  (cs/union a b))

 (doseq [klass [PersistentHashSet PersistentTreeSet]]
+  (defmethod g/zero? [klass] [s] (empty? s))


I'm counting on our tests to catch my tired eyes as I read these implementations... this is heroic work!

"Refactor in the presence of running tests" has never felt so relevant!!

src/emmy/differential.cljc

sritchie · 2023-10-12T22:10:06Z

src/emmy/expression/analyze.cljc

@@ -19,6 +19,7 @@
  an expression with respect to an analyzer is therefore effected by a
  round-trip to and from the canonical form."
  (:require [emmy.expression :as x]
+            [emmy.generic :as g]
            [emmy.numsymb :as sym]
            [emmy.util :as u]
            [emmy.value :as v]))


can this import go away or no?

there's a ref. to g/freeze and v/integral?

Some of the utility level stuff in v/ could move to g/, and then the import of v would occur less often. Willing to consider that

sritchie · 2023-10-12T22:13:24Z

src/emmy/ratio.cljc

@@ -12,7 +12,7 @@
  (:refer-clojure :exclude [ratio? numerator denominator rationalize])
  (:require #?(:clj [clojure.core :as core])
            #?(:clj [clojure.edn] :cljs [cljs.reader])
-            #?(:cljs ["fraction.js/bigfraction.js$default" :as Fraction])
+            #?(:cljs ["fraction.js/bigfraction.js" :as Fraction])


interesting, why do JS people keep messing with their builds...

I did notice that (1) in the current version, there's an exports block in fraction.js's package.json, (2) bigfraction.js is not listed there, and (3) I still don't know why this was ever a problem on GitHub, where the older version had no exports clause and I guess was relying on a permissive interpretation.

I wanted to get "everyone onto the same page" and so experimented with adding and removing $default. I don't even know what it's really for, I was just trying to shake things into place.

I'm quite open to the idea of just stashing bigfraction.js somewhere in the tree and forgetting about it

sritchie · 2023-10-12T22:18:00Z

src/emmy/simplify.cljc

@@ -74,7 +74,7 @@
        expr
        (let [canonicalized-expr (canonicalize new-expr)]
          (cond (= canonicalized-expr expr) expr
-                (v/zero?
+                (g/numeric-zero?


This is the one where what's being tested for zero is a lazy seq returned from simplify that got me wondering about this... I wanted to avoid the idea that a realized empty list could spoof this. I can put it back if you prefer

BTW, I think what you had originally written is perfectly safe, in hindsight

sritchie · 2023-10-13T18:59:31Z

src/emmy/value.cljc

-  (freeze [v] (:name (meta v)))
-  (exact? [_] false)
-  (kind [v] (type v))
+;;  Var


curious what you're thinking here, are these caught by any other line?

Currently we just have kind for nil (returning nil). Since nil no longer gets any of the other formerly-Value methods, I thought, should it have a kind?

If nil has no kind, i.e., it's not an Emmy value at all, a few things break. We lose this:

(defmethod g/partial-derivative [t nil] [f _] (multivariate f [])))

Although I think that could be fixed by having the (kind nil) be ::v/seqtype. There are a few other cases of error that we could fix.

But the experiment of removing IKind from nil has kind of convinced me that we want the kind function to be a total function, safe to call on any Clojure(Script) value, so that a defmethod could in principle be written for any object.

So I am comfortable with the status quo. Not having the other Value methods like zero? defends against unfortunate nil-puns.

sritchie · 2023-10-13T19:00:20Z

src/emmy/value.cljc

@@ -331,7 +246,7 @@

     IPrintWithWriter
     (-pr-writer [x writer _]
-       (let [rep (if (<= x (.-MAX_SAFE_INTEGER js/Number))
+       (let [rep (if (< (if (< x 0) (- x) x) (.-MAX_SAFE_INTEGER js/Number))


good catch! I might recommend a test for this if it didn't make it in yet, which it might have, apologies for my code nav on this PR

Something definitely breaks w/o this :)

What has happened, I think, is that there were some "overlapping" cases in the Value regime that have been clarified by the transition to defmulti, which focused my attention on the (very few) bugs that I found

sritchie · 2023-10-13T19:01:57Z

@littleredcomputer looking good pending the few comments that I left... thank you for doing this!!

littleredcomputer · 2023-10-14T02:01:50Z

@littleredcomputer looking good pending the few comments that I left... thank you for doing this!!

I'm really happy to do this

I also added a console.log to the fraction patch: we're going to learn more about how shadow-cljs deals with things like that

littleredcomputer · 2023-10-26T21:37:13Z

Finally have enough coverage. Now on to fixing the fraction.js thing and we're done.

littleredcomputer added 9 commits September 29, 2023 17:00

Move zero? from protocol to multifn

03fe1b1

retire Value protocol members one?, identity?

4e676e0

zero-like, one-like, identity-like moved from Value protocol to multifns

0e0fb0a

Level up Clojurescript

5fa2a53

exact? and freeze (clojure only).

9ff9a82

Next step (after cljs level-up) is to rename the remaining one-method Value protocol to IKind.

cljs level up (almost: there are two test case failures having to do …

afbbb05

…with cljs numeric tower, but this puts things back into semi-usable state for the night)

cljs bug fixes

efa73e0

some of these are interesting... how were they working before?

rename Value to IKind

186cdaf

doc tweaks

9d8049d

littleredcomputer added 5 commits October 11, 2023 10:44

add some more test coverage

f37e03e

tweaks

bf86cf5

tweaks

c5d86cf

fix

9840eae

try to get bigfraction to work in CI (it works on my desktop, but...)

3099424

littleredcomputer requested a review from sritchie October 11, 2023 21:10

littleredcomputer added 2 commits October 11, 2023 15:56

more coverage

d1cf4b3

use a patch-package to get bigfraction.js to import cleanly for cljs

b48789f

sritchie reviewed Oct 12, 2023

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

sritchie reviewed Oct 12, 2023

View reviewed changes

sritchie approved these changes Oct 13, 2023

View reviewed changes

sritchie changed the title ~~Littleredcomputer/retire value protocol~~ Retire value protocol in favor of multimethods Oct 13, 2023

littleredcomputer added 6 commits October 17, 2023 14:03

coverage.

2d75ffa

I also added a console.log to the fraction patch: we're going to learn more about how shadow-cljs deals with things like that

code coverage

496a3d1

doc, coverage

8d13acc

cov

ec33728

coverage

e35d090

coverage

316d36c

littleredcomputer added 3 commits October 26, 2023 11:31

coverage

49e88e6

coverage

5faf68c

remove literal ratios for cljs

b171645

littleredcomputer added 2 commits October 27, 2023 16:15

pin fraction.js to 4.2.1; remove patch stuff. Will this pass CI?

c104511

add PR number to changelog entry

49a92a3

littleredcomputer merged commit a3a4954 into main Oct 28, 2023
6 checks passed

littleredcomputer deleted the littleredcomputer/retire-value-protocol branch October 28, 2023 22:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Retire value protocol in favor of multimethods #149

Retire value protocol in favor of multimethods #149

littleredcomputer commented Oct 10, 2023

codecov bot commented Oct 10, 2023 •

edited

Loading

sritchie left a comment

sritchie Oct 12, 2023

littleredcomputer Oct 14, 2023

sritchie Oct 12, 2023

littleredcomputer Oct 14, 2023

littleredcomputer Oct 14, 2023

sritchie Oct 14, 2023

sritchie Oct 12, 2023

littleredcomputer Oct 14, 2023

sritchie Oct 12, 2023

littleredcomputer Oct 14, 2023

sritchie Oct 12, 2023

littleredcomputer Oct 14, 2023

sritchie Oct 12, 2023

littleredcomputer Oct 14, 2023

sritchie Oct 12, 2023

littleredcomputer Oct 14, 2023

littleredcomputer Oct 14, 2023

sritchie Oct 13, 2023

littleredcomputer Oct 14, 2023

sritchie Oct 13, 2023

littleredcomputer Oct 14, 2023

sritchie commented Oct 13, 2023

littleredcomputer commented Oct 14, 2023

littleredcomputer commented Oct 26, 2023

		@@ -230,9 +220,9 @@
		litfns)))

		(u/sci-macro with-literal-functions [litfns & body]

Retire value protocol in favor of multimethods #149

Retire value protocol in favor of multimethods #149

Conversation

littleredcomputer commented Oct 10, 2023

codecov bot commented Oct 10, 2023 • edited Loading

Codecov Report

sritchie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sritchie commented Oct 13, 2023

littleredcomputer commented Oct 14, 2023

littleredcomputer commented Oct 26, 2023

codecov bot commented Oct 10, 2023 •

edited

Loading