Add Ord spec #235

gabejohnson · 2017-03-25T21:01:27Z

Closes #233

I intentionally left the definition of Ordering implicit (as is that of Boolean under Setoid) to spur discussion.

I'm looking for any suggestions on a better way to state the laws in both the README and the tests.

davidchambers · 2017-03-25T21:05:25Z

README.md

+A value that implements the Ord specification must also implement
+the [Setoid](#setoid) specification.
+
+1. `a.compare(a) <= 0` (reflexivity)


Why <= rather than ===?

I was simply copying the laws from PureScript. No other reason. === works just as well and is probably clearer.

davidchambers · 2017-03-25T21:06:46Z

README.md

+    1. If `b` is not the same Ord, behaviour of `compare` is
+       unspecified (returning NaN is recommended).
+
+2. `compare` must return a number (-1, 0 or 1).


I suggest backticks around -1, 0, and 1 (and NaN). Also, an Oxford comma. ;)

joneshf · 2017-03-26T03:30:11Z

I'm not enamored with the idea of using -1, 0 and 1. There are other formulations for orderings, posets and comparisons we can use that don't require this C-style obfuscation.

gabejohnson · 2017-03-26T03:40:57Z

@joneshf what would you suggest? Strings? Symbols?

gabejohnson · 2017-03-26T04:31:03Z

Okay here's another formulation:

a.compare(a).equals([a, a]) (reflexivity)
If a.compare(b).equals([b, a]) and b.compare(a).equals([a, b]), then a.equals(b) === true (antisymmetry)
If a.compare(b).equals([b, a]) and b.compare(c).equals([c, b]), then a.compare(c).equals([c, a]) (transitivity)

safareli · 2017-03-26T09:22:58Z

dependencies.png should be regenerated

gabejohnson · 2017-03-27T03:50:45Z

How about:

a.covers(a) === false (irreflexivity)
a.covers(b) !== b.covers(a) (asymmetry)
If a.covers(b) and b.covers(c), then a.covers(c) (transitivity)

// defined like operators
const lt = (x, y) => x.covers(y);
const gte = (x, y) => !x.covers(y);
const lte = (x, y) => x.covers(y) || x.equals(y);
const gt = (x, y) => !x.covers(y) && !x.equals(y);
const min = (x, y) => x.covers(y) ? x : y;
const max = (x, y) => x.covers(y) ? y : x;

const nativeCompare = (x, y) => x.covers(y) ? -1 : x.equals(y) ? 0 : 1;

Edit: covers is the wrong name. c.covers(a) if there's no b such that a < b < c.

gabejohnson · 2017-03-27T04:17:04Z

Back to using <=:

a.precedes(a) === true (reflexivity);
If a.precedes(b) and b.precedes(a), then a.equals(b) (antisymmetry)
If a.precedes(b) and b.precedes(c), then a.precedes(c) (transitivity)

// defined like operators
const equals = (x, y) = x.precedes(y) && y.precedes(x);
const lte = (x, y) => x.precedes(y);
const gt = (x, y) => !x.precedes(y);
const lt = (x, y) => x.precedes(y) && !y.precedes(x);
const gte = (x, y) => !x.precedes(y) || y.precedes(x);
const min = (x, y) => x.precedes(y) ? x : y;
const max = (x, y) => x.precedes(y) ? y : x;

const nativeCompare = (x, y) => !x.precedes(y) ? 1 : y.precedes(x) ? 0 : -1;

gabejohnson · 2017-03-27T18:43:50Z

I just changed the formulation to use precedes instead of compare. Added a derivation for equals and regenerated dependencies.png.

davidchambers · 2017-03-27T18:58:24Z

README.md

@@ -605,6 +632,11 @@ The `profunctor` method takes two arguments:
 When creating data types which satisfy multiple algebras, authors may choose
 to implement certain methods then derive the remaining methods. Derivations:

+  - [`equals`][] may be derived from [`precedes`][]:
+    ```js


Nit: Empty line before ```js, please.

davidchambers · 2017-03-27T19:49:01Z

I'll defer to @joneshf, @safareli, and others who are actually familiar with this stuff, but I will say that in its current state this pull request seems extremely elegant!

safareli · 2017-03-28T13:27:27Z

laws/ord.js

+  return eq(a, d) && eq(b, d) && eq(c, d);
+};
+
+const transitivity = t => eq => x => {


Instead of passing t-s I think we should pass values on which precedes will be called.
Like just pass f, g, h instead of t and x.
Same in other laws.

I defined precedes on Id which may have been wrong as it should just delegate to value's precedes. Should I define it in internal/func.js instead? I could implement it for String and Number and delegate otherwise.

I think yes

safareli · 2017-03-28T13:38:26Z

Small issue with laws otherwise LGTM.
/cc @SimonRichardson @puffnfresh

gabejohnson · 2017-03-28T16:03:08Z

Updated with changes to tests.

davidchambers · 2017-04-02T11:05:58Z

Are you happy with this pull request in its current state, @joneshf? If so, let's merge it and release it as v3.2.0.

sadasant · 2017-04-02T16:34:36Z

❤️

scott-christopher · 2017-04-02T23:41:55Z

I would have assumed precedes meant < rather than <=. e.g. (42).precedes(42) === true doesn't sound right to me. How about lte as an alternative?

Besides naming, I welcome this addition.

davidchambers · 2017-04-02T23:52:21Z

I like @scott-christopher's suggestion. Let's use lte rather than precedes.

joneshf · 2017-04-03T02:15:26Z

I have a different qualm about naming. The name Ord comes from Haskell98. That typeclass is for total orderings. But here we're not requiring a total ordering. Although we don't really follow the typeclasses for other things (Setoid is a thing, Monad doesn't have methods, etc), I wonder how confusing this will be for someone coming from Haskell. So, I guess we should mention whether we want a partial ordering or a total ordering:

Rename to something like Poset, and mention that this is for partial orderings. Since we're actually specifying a poset here, might as well name it appropriately.
Keep the current nameOrd, change the first law from reflexivity to totality, and mention that this is for total orderings.

I'm fine with either of those options. However, I think we should seriously reconsider it being unspecified currently.

davidchambers · 2017-04-03T02:19:42Z

Thanks for the feedback, Hardy. :)

However, I think we should seriously reconsider it being unspecified currently.

I don't understand this sentence. What is it that's unspecified?

joneshf · 2017-04-03T02:22:13Z

I guess nothing, thinking about it. You can infer that it's for partial orderings and not total orderings based on the laws.

I think what I should have said was that we should be explicit, rather than saying that something was unspecified 😊.

davidchambers · 2017-04-03T02:38:04Z

I'm in favour of Hardy's second option as I believe the intention was to define the type class of totally ordered sets. Do you agree, @gabejohnson?

gabejohnson · 2017-04-03T13:47:36Z

@davidchambers @joneshf that was an oversight on my part. I'll make it a total ordering unless you can think of a reason we wouldn't want to add that constraint.

gabejohnson · 2017-04-03T13:57:47Z

@scott-christopher @davidchambers I was looking for a name that didn't suggest quantity and precedes is already used as a synonym for lte in some of the work I read, albeit inconsistently. I also thought it sounded nice paired with equals.
That said, since a total order can have a "least element" I suppose "less than" doesn't have to suggest quantity. I'll change it to lte unless anyone can think of a better name.

gabejohnson · 2017-04-04T12:32:17Z

I've implemented all of the requested changes. Everything look good?

davidchambers · 2017-04-04T12:32:23Z

index.js

@@ -22,7 +22,8 @@
    extend: 'fantasy-land/extend',
    extract: 'fantasy-land/extract',
    bimap: 'fantasy-land/bimap',
-    promap: 'fantasy-land/promap'
+    promap: 'fantasy-land/promap',
+    lte: 'fantasy-land/lte'


Let's include this after equals.

gabejohnson · 2017-04-04T12:34:16Z

Done.

davidchambers · 2017-04-04T12:35:30Z

README.md

+  - [`equals`][] may be derived from [`lte`][]:
+
+    ```js
+    function (b) { return this.lte(b) && b.lte(this); }


For consistency with the other derivations, let's drop the space after function.

I'd like to see the parameter named other rather than b to suggest that the method expects an argument of the same type.

davidchambers · 2017-04-04T12:35:50Z

README.md

@@ -700,6 +733,7 @@ be equivalent to that of the derivation (or derivations).
 [`extract`]: #extract-method
 [`map`]: #map-method
 [`of`]: #of-method
+[`lte`]: #lte-method


davidchambers · 2017-04-04T12:41:14Z

laws/ord.js

+  const b = g[lte](f);
+  const c = true;
+  return eq(a || b, c);
+};


Taking eq strikes me as unnecessary. I'd find this clearer:

const totality = a => b => a[lte](b) || b[lte](a);

I think all of the laws are like this—equals included. I can't remember why, but there was a reason we switched to this way of writing the laws.

I.e., it seems fine to me to have these laws work similarly, and revisit whether that's appropriate in another PR. But, if you feel strongly, I won't fight it.

Given that I'm working on fantasyland/fantasy-laws#1, I'd very much like to know the reason. If it occurs to you, let me know. :)

Let's leave this as it is, Gabe. These functions will be removed from this repository in due course.

Nice! I didn't know that existed.

gabejohnson · 2017-04-04T12:57:13Z

I'll squash as soon as you sign off.

davidchambers · 2017-04-04T13:12:19Z

Let's update the height of the <img> for dependencies.png. The image's natural dimensions are now 1047 × 347. The width is fixed at 888; the height should be (888 / 1047) * 347 ≈ 294. ;)

gabejohnson · 2017-04-04T13:17:17Z

Done

davidchambers

I'm very excited about this addition! 🌳

gabejohnson · 2017-04-04T13:48:43Z

@davidchambers do you want me to squash it?

davidchambers · 2017-04-04T15:39:14Z

Yes, it would be nice to squash the commits. :)

* Use precedes instead of compare to define Ord * Replace reflexivity with totality so we can have a toset * Change 'precedes' to 'lte'

gabejohnson · 2017-04-04T16:12:37Z

Squashed

gabejohnson · 2017-04-04T17:08:33Z

Looks like I forgot to put the link to the new spec in the list here

davidchambers · 2017-04-04T17:17:44Z

No worries. Would you mind opening another pull request? I'll then merge it and release v3.2.0.

davidchambers reviewed Mar 25, 2017

View reviewed changes

gabejohnson force-pushed the add-ord branch 2 times, most recently from 0821ddd to 86c97e5 Compare March 27, 2017 18:40

davidchambers reviewed Mar 27, 2017

View reviewed changes

gabejohnson force-pushed the add-ord branch from 86c97e5 to df7408e Compare March 27, 2017 19:16

gabejohnson force-pushed the add-ord branch from df7408e to 78d2a50 Compare March 27, 2017 19:56

safareli reviewed Mar 28, 2017

View reviewed changes

davidchambers mentioned this pull request Mar 28, 2017

Add S.sort and S.sortBy sanctuary-js/sanctuary#353

Closed

davidchambers mentioned this pull request Apr 2, 2017

ord: add comparison functions sanctuary-js/sanctuary#239

Closed

davidchambers reviewed Apr 4, 2017

View reviewed changes

gabejohnson force-pushed the add-ord branch from 8a5f1b6 to e3f0bbb Compare April 4, 2017 12:34

davidchambers reviewed Apr 4, 2017

View reviewed changes

gabejohnson force-pushed the add-ord branch from 320ffbe to 6afbd29 Compare April 4, 2017 13:17

davidchambers approved these changes Apr 4, 2017

View reviewed changes

Add Ord spec

5a00508

* Use precedes instead of compare to define Ord * Replace reflexivity with totality so we can have a toset * Change 'precedes' to 'lte'

gabejohnson force-pushed the add-ord branch from 6afbd29 to 5a00508 Compare April 4, 2017 16:11

davidchambers merged commit 3af8d23 into fantasyland:master Apr 4, 2017

gabejohnson mentioned this pull request Apr 5, 2017

Defining Ord in terms of lt vs lte #239

Closed

gabejohnson deleted the add-ord branch January 11, 2018 19:39

rpominov mentioned this pull request Apr 11, 2018

Ord compare fantasyland/static-land#49

Closed

Add Ord spec #235

Add Ord spec #235

Conversation

gabejohnson commented Mar 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joneshf commented Mar 26, 2017

gabejohnson commented Mar 26, 2017

gabejohnson commented Mar 26, 2017

safareli commented Mar 26, 2017

gabejohnson commented Mar 27, 2017 • edited Loading

gabejohnson commented Mar 27, 2017 • edited Loading

gabejohnson commented Mar 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidchambers commented Mar 27, 2017

safareli Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

safareli commented Mar 28, 2017

gabejohnson commented Mar 28, 2017

davidchambers commented Apr 2, 2017

sadasant commented Apr 2, 2017

scott-christopher commented Apr 2, 2017

davidchambers commented Apr 2, 2017

joneshf commented Apr 3, 2017

davidchambers commented Apr 3, 2017

joneshf commented Apr 3, 2017

davidchambers commented Apr 3, 2017

gabejohnson commented Apr 3, 2017

gabejohnson commented Apr 3, 2017 • edited Loading

gabejohnson commented Apr 4, 2017

Choose a reason for hiding this comment

gabejohnson commented Apr 4, 2017

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

gabejohnson commented Apr 4, 2017

davidchambers commented Apr 4, 2017

gabejohnson commented Apr 4, 2017

davidchambers left a comment

Choose a reason for hiding this comment

gabejohnson commented Apr 4, 2017

davidchambers commented Apr 4, 2017

gabejohnson commented Apr 4, 2017

gabejohnson commented Apr 4, 2017

davidchambers commented Apr 4, 2017

gabejohnson commented Mar 27, 2017 •

edited

Loading

gabejohnson commented Mar 27, 2017 •

edited

Loading

safareli Mar 28, 2017 •

edited

Loading

gabejohnson commented Apr 3, 2017 •

edited

Loading