Improve regular_nested example for “Polymorphism” chapter of the manual

[steffahn]

Previously, the example dept function for regular_nested was clearly buggy. In particular the expression

1 + max (maximal_depth a) (maximal_depth (Nested q))

was adding 1 to the maximal_depth (Nested q) side, too, resulting in unbalanced depth calculation (later list elements get increasingly larger “depth”)

The minimal fix would have been

max (1 + maximal_depth a) (maximal_depth (Nested q))

but then I still find the Nested [] -> 0 case confusing. Adding a List [] element to produce Nested [List []] makes the depth jump by 2!?

I could just re-define this as Nested [] -> 1, but I believe that a mutually recursive definition is even easier to understand.

In particular, now the new

let rec regular_depth = function
  | List _ -> 1
  | Nested n -> 1 + max_regular_depth n
and …;;

is structurally (at least superficially) very similar to

let rec depth = function
  | List _ -> 1
  | Nested n -> 1 + depth n;;

which helps readers focus less on wondering if the latter really is just the result of directly “adapting” the former.

The naming is also changed to keep more consistent: now it is regular_nested with regular_depth and then nestedwithdepth`

Finally, there also was a clear type; the original text had one instance where it misspelled “regular_nested” as “regular_depth” in line 306 (now 308).

(This PR also includes a few instances of bad-looking extra indentation removed, and the definition of type 'a regular_nested is not separated via ;; from the example let l = ... which should help improve readability)

[Octachron]

I agree that it is a good idea to simplify the specification of this function, and it is also a good idea to have the function name reflect its type. I have few minor comments on how we could simplify it further that you might want to consider. Thanks for the PR !

[steffahn]

Sure…

Regarding fold_left, that’s of course an alternative.
Here are the alternatives so far:

Currently in the PR:

let rec regular_depth = function
  | List _ -> 1
  | Nested n -> 1 + max_regular_depth n
and max_regular_depth = function
  | [] -> 0
  | a::q -> max (regular_depth a) (max_regular_depth q);;

using fold_left

let rec regular_depth = function
  | List _ -> 1
  | Nested n -> 1 + List.fold_left (fun acc a -> max acc (regular_depth a)) 0 n;;

using fold_left and map (avoids fun … -> …)

let rec regular_depth = function
  | List _ -> 1
  | Nested n -> 1 + List.fold_left max 0 (List.map regular_depth n);;

the original code with minimal changes (new name, 1 + moved in, -> 0 became -> 1)

let rec regular_depth = function
  | List _ -> 1
  | Nested [] -> 1
  | Nested (a::q) -> max (1 + regular_depth a) (regular_depth (Nested q));;

---

Regarding the List _ -> 1 base case, well… the specification might be simpler if it was 0, but with -> 1 it keeps better (superficial) similarity with the subsequent depth function.

Arguably, one could even consider to use a base case of single element

type 'a regular_nested = Elem of 'a | Nested of 'a regular_nested list

type 'a nested = Elem of 'a | Nested of 'a list nested;;

which helps also in that the list type constructor only appears once.
But then Elem [or Item, or Leaf] is a bad name for the base case of 'a nested
just consider expressions such as Nested(Nested(Elem [ [7]; [8] ]))
I can’t come up with a name that would let us get away with the superficial similarity again; really the meaning of the constructors for 'a nested are more something like

type 'a nested = BaseCase of 'a | MoreNesting of 'a list nested;;

(Etc… Then one could define the regular_depth using Elem _ -> 0 and the depth using BaseCase _ -> 0.)

But IMO any superficial similarity here helps avoid distracting from the main point of this section, which is not about how to implement a nested list data structure correctly. Also such a change would involve refactoring … well …everything below, which is going way too far.

---

Though I get your arguments, especially Nested _ always being considered deeper than List _ would be nice…

We can’t simply define a base case of List _ -> 0 for both 'a regular_nested and 'a nested, because there’s good plain text description in the latter case ('a nested) already

Intuitively, a value of type 'a nested is a list of list …of list of elements a with k nested list. We can then adapt the maximal_depth function defined on regular_depth into a depth function that computes this k.

And I would really like for something like depth ( Nested(List [ [7]; [8] ]) )
to still match regular_depth ( Nested [ List [7]; List [8] ] )

Perhaps, it’s workable to define it as “maximal number of Nested plus 1”?

Reiterating the alternatives, with that specification they’d now look as follows:

0 became 1 in max_regular_depth []

let rec regular_depth = function
  | List _ -> 1
  | Nested n -> 1 + max_regular_depth n
and max_regular_depth = function
  | [] -> 1
  | a::q -> max (regular_depth a) (max_regular_depth q);;

0 became 1 for the fold_left

let rec regular_depth = function
  | List _ -> 1
  | Nested n -> 1 + List.fold_left (fun acc a -> max acc (regular_depth a)) 1 n;;

let rec regular_depth = function
  | List _ -> 1
  | Nested n -> 1 + List.fold_left max 1 (List.map regular_depth n);;

-> 1 became -> 2 for Nested []

let rec regular_depth = function
  | List _ -> 1
  | Nested [] -> 2
  | Nested (a::q) -> max (1 + regular_depth a) (regular_depth (Nested q));;

---

So that’s 2*4 versions now, which would you prefer? ;-)

[Octachron]

I agree with the argument that we want the two depths function to agree on inputs that are morally equivalent. In this case, I think that your idea to keep *depth (List _ ) = 1 and define depth (Nested [ ]) as 2 is fine.

Concerning the implementation, I think we want to keep it as simple as possible since like you said the implementation is not the point (we just want it to be coherent to not nerdsnipe readers). Thus I am hesitating between

let rec regular_depth = function
  | List _ -> 1
  | Nested [] -> 2
  | Nested (a::q) -> max (1 + maximal_depth a) (maximal_depth (Nested q));;

which has this weird Nested [] base case, and slightly not-idiomatic inline implementation of fold or

let rec regular_depth = function
 | List _ -> 1
 | Nested n -> 1 + List.fold_left max 1 (List.map regular_depth n);;

which involves few more list functions in the definition.

After writing this comment, I have a slight preference for the second version but I will let you choose your preferred option.

[steffahn]

Great! When I finished writing my previous answer,

let rec regular_depth = function
  | List _ -> 1
  | Nested n -> 1 + List.fold_left max 1 (List.map regular_depth n);;

was my preference, too (which I didn’t note to let you give an unbiased answer)

[Octachron]

@steffahn , would you mind adding a Changes entry in the Manual and documentation section for 5.3.0 ?

[steffahn]

Added an entry… Ah, you said under 5.3.0, so should the entry be further down?

[Octachron]

It should but this is not an issue: I will move the entry to the right location when cherry-picking the change to the 5.3 branch.

[Octachron]

Merged, changes entry moved, and cherry-picked to 5.3 in 8f12f9c. Thanks again for the patch !

[steffahn]

thank you for the quick and responsive review :-)