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

move literal-function => abstract, switch fn inheritance order #171

Merged
merged 67 commits into from
Dec 8, 2020

Conversation

sritchie
Copy link
Member

@sritchie sritchie commented Dec 1, 2020

This PR

  • switches the inheritance direction of ::v/function and ::sicmutils.function
  • moves all of the literal-functionmachinery and tests into their own namespace, sicmutils.abstract.function
  • keeps sicmutils.function quite clean. This gets the library closer to a "built from Clojure up" style.

::v/functionis now the main thing, and::sicmutils.function/function` extends from this.

@sritchie sritchie changed the title Switch fn inheritance, make abstract fn extend ::v/function [in progress] Switch fn inheritance, make abstract fn extend ::v/function] Dec 3, 2020
@sritchie sritchie changed the title Switch fn inheritance, make abstract fn extend ::v/function] Switch fn inheritance, make abstract fn extend ::v/function Dec 3, 2020
@codecov-io
Copy link

codecov-io commented Dec 3, 2020

Codecov Report

Merging #171 (dbeb7f3) into sritchie/abstract_updates (95bfc37) will increase coverage by 0.08%.
The diff coverage is 96.87%.

Impacted file tree graph

@@                      Coverage Diff                      @@
##           sritchie/abstract_updates     #171      +/-   ##
=============================================================
+ Coverage                      84.41%   84.50%   +0.08%     
=============================================================
  Files                             73       73              
  Lines                           6858     6859       +1     
  Branches                         394      393       -1     
=============================================================
+ Hits                            5789     5796       +7     
+ Misses                           675      670       -5     
+ Partials                         394      393       -1     
Impacted Files Coverage Δ
src/sicmutils/function.cljc 84.74% <94.11%> (-0.07%) ⬇️
src/sicmutils/calculus/derivative.cljc 92.73% <100.00%> (+0.02%) ⬆️
src/sicmutils/calculus/vector_field.cljc 98.38% <100.00%> (ø)
src/sicmutils/mechanics/hamilton.cljc 95.68% <100.00%> (ø)
src/sicmutils/operator.cljc 83.33% <100.00%> (ø)
src/sicmutils/series.cljc 59.13% <100.00%> (+0.17%) ⬆️
src/sicmutils/numsymb.cljc 92.66% <0.00%> (ø)
src/sicmutils/polynomial/gcd.cljc 90.17% <0.00%> (+2.31%) ⬆️
src/sicmutils/util.cljc 78.57% <0.00%> (+3.57%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95bfc37...dbeb7f3. Read the comment docs.

@sritchie sritchie changed the title Switch fn inheritance, make abstract fn extend ::v/function move literal-function => abstract, switch fn inheritance order Dec 3, 2020
;; along with this code; if not, see <http://www.gnu.org/licenses/>.
;;

(ns sicmutils.abstract.function
Copy link
Member Author

Choose a reason for hiding this comment

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

just a move, nothing new.

@@ -110,6 +110,13 @@
(x/expression-of expr)
expr))

(defn- defunary [generic-op op-sym]
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 moved this up to keep unary => binary order...

@@ -431,14 +432,11 @@
((d f) (first xs))
((d #(apply f %)) (matrix/seq-> xs)))))))

(derive ::x/numeric ::codiff)
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 had introduced this and forgotten that ::scalar was all we really need, no need now for a new ::codiff.

@@ -493,11 +491,14 @@
(defmethod g/partial-derivative [::matrix/matrix v/seqtype] [f selectors]
(multivariate-derivative f selectors))

(def derivative-symbol 'D)
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 moved this in here so it lives in this one spot.

@@ -31,6 +32,8 @@

(declare generate)

(derive ::matrix ::f/cofunction)
Copy link
Member Author

Choose a reason for hiding this comment

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

@littleredcomputer , curious what you think here. I realized that if we were building this up from scratch, we wouldn't have matrices at the point where we added functions... so I moved this derivation over to matrix, so that function.cljc could stay cleaner.

What do you think?

If someone ever wants to use a matrix, they have to require this ns anyway, which will trigger the derive call. And if they don't, no sweat.

@@ -563,6 +563,7 @@

(derive ::x/numeric ::coseries)
(derive ::v/number ::coseries)
(derive ::v/function ::coseries)
Copy link
Member Author

Choose a reason for hiding this comment

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

same thought here as in matrix; series should register themselves.

(derive ::up ::structure)
(derive ::down ::structure)
(derive PersistentVector ::up)

;; Structures can interact with functions.
(derive ::structure ::f/cofunction)
Copy link
Member Author

Choose a reason for hiding this comment

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

same thought here.

;; along with this code; if not, see <http://www.gnu.org/licenses/>.
;;

(ns sicmutils.abstract.function-test
Copy link
Member Author

Choose a reason for hiding this comment

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

these are all the tests that explicitly use literal-function.

[sicmutils.generic :as g]
[sicmutils.generators :as sg]
[sicmutils.matrix :as m]
Copy link
Member Author

Choose a reason for hiding this comment

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

The only tests left here are the ones that did NOT use literal-function. Now we can add more, only about Clojure functions.

Copy link
Collaborator

@littleredcomputer littleredcomputer left a comment

Choose a reason for hiding this comment

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

LGTM

@sritchie sritchie changed the base branch from sritchie/abstract_updates to master December 8, 2020 12:16
@sritchie sritchie merged commit 592642a into master Dec 8, 2020
@sritchie sritchie deleted the sritchie/switch_fn_inheritance branch December 8, 2020 13:00
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

3 participants