Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Removed the Enum instance for Dimensional. #27

Closed
wants to merge 1 commit into from

2 participants

@dmcclean

It seems like the Enum instance needlessly exposes the implementation details of Dimensional, because there is no physical reason why succ (1 *~ meter) should be 2 meters, while succ (1 *~ foot) is 1.3048 meters.

Perhaps some analog of enumFromTo that increments by a specified quantity might better suit any existing clients? Or [0..1000] *~~ meter?

@bjornbm
Owner

Hmm... I use this quite a bit. Mostly for time series, e.g. [t0, t0+dt .. t1], although I could sacrifice syntax convenience and use an enumFromTo analog. But I'm pretty sure that I at some point needed to do something with an Enum constraint although I cannot recall exactly what.

It is a bit of a wart, but I suppose I was able to justify it to myself by the arguing that the natural increment for a type revolving around the SI is by 1 [SI base unit]. See also the announcement.

@dmcclean

The announcement raises the same alternative of *~~, which I really think is the way to go.

If interacting with something else that is a little weird absolutely requires an Enum instance, and a newtype isn't desired, I would say that I think clients could define one themselves. I'm pretty sure they could even define it without access to the Dimensional constructor, if it was for a monomorphic Dimension. A client with this problem could also use -XStandaloneDeriving (I'm not sure if doing so requires importing the Dimensional constructor, and I can't check because I left my laptop with GHC 7.8 at home).

instance Enum Double is a misfeature in it's own right. I'm not the only one who thinks so, see this long haskell-cafe thread for some examples of why.

@bjornbm
Owner

*~~ wouldn't work for my time series example. If we provide a enumFromThenTo analog I think the simplest Enum instance would be the trivial (haven't looked at code or compiled):

instance Enum a => Enum (Time a) where enumFromThenTo = enumFromThenToAnalog

So I agree clients could define it themselves. ;)

I'm leaning towards agreeing with you. I'll try to find my Enum requirement and whether I can get around it with e.g. a newtype which I agree would be the best solution.

@dmcclean

Actually, speaking of syntactic convenience, does -XRebindableSyntax rebind the desugaring of [t0, t0+dt .. t1]?

@bjornbm
Owner

I believe it does. The problem is that you would no longer be able to do [1, 2 .. 1000] *~~ meter instead. :(

@dmcclean

I just checked and it doesn't in 7.6.

@dmcclean

Mostly tongue-in-cheek proposal:

instance Num a => Enum (Quantity d a) where
  enumFromThen = ...
  enumFromThenTo = ...
  succ = error "..."
  pred = error "..."
  enumFrom = error "..."
  fromEnum = error "..."
  toEnum = error "..."
@dmcclean

until (> tf) (+ dt) ti isn't so bad either. I wonder if it even has numerical advantages, because you pass dt explicitly, rather than implicitly as (ti + dt) - ti, but that isn't my area of expertise?

@bjornbm
Owner

Take a look at 9a03614. Am I being too helpful providing all those trivial functions?

@dmcclean

Hmm, interesting question.

I think they are generally useful, and the names they are taking aren't names that are going to be overlapping with anything, so I think it is good to have them. Having the documentation for them should serve to avoid what might otherwise be a FAQ. And I can't think of any logical submodule name where it would make sense to move them. So I vote to keep them.

I don't personally expect to have much use for them, because all my time-stepping/integration is wrapped up with a lot of complicated signal function machinery, but certainly they are helpful for either more complicated (hence more explicit) or simpler uses.

@bjornbm
Owner

With regards to your comment on 9a03614: I've changed fromThenTo (actually fromIncrTo) to be consistent with enumFromThenTo. (At least my QuickCheck test tells me they are consistent.) See aadffc6

But even with the explicit step size enumFromThenTo is quite bananas with regards to the upper (non-)limit, as Casey McCann says:

the "enum to" value at the end behaves neither as an upper bound (the
sequence may exceed it in an effort to avoid rounding errors) nor as a
final element (it may not be in the sequence at all, even if it has an
exact floating point representation). This seems needlessly confusing
to me and is arguably broken no matter which way you slice it.

The question is, what semantics do we want, and are we qualified to “know better” than the Prelude? I myself would expect xf to be an upper bound, but would we want to worry about FP rounding errors causing (a value close to) xf to being included or not in the sequence seemingly randomly?

Or should we drop the …To functions completely and let clients shoot themselves in the foot without our help?

@dmcclean

Good question.

I'd be inclined to say that we keep it up to the last value that is strictly <= the end point. Anyone who wants the exact last number is probably barking up the wrong tree anyway?

I'd personally probably want the one with xi, dx, and n or the one with xi, xf, and n anyway, neither of which seems to have a prelude analogue.

@bjornbm
Owner

Do you mean?:

nFromIncr :: (Ord a, Fractional a)
          => Int -> Quantity d a -> Quantity d a -> [Quantity d a]
nFromIncr n xi dx = take (n Prelude.+ 1) $ fromIncr xi dx

nFromTo :: (Ord a, Fractional a)
        => Int -> Quantity d a -> Quantity d a -> [Quantity d a]
nFromTo 0 xi _  = [xi]
nFromTo n xi xf = (++ [xf])  -- Avoid rounding errors on final element.
                $ take n $ fromIncr xi ((xf - xi) / (fromIntegral n *~ one))

Or should n be the total number of elements in the result rather than the number of steps taken from xi?

Would e.g. fromIncrN xi dx n and fromToN xi xf n be better naming and argument ordering?

@dmcclean

I was thinking it's the total number of elements in the result, but the other way might be better. If you do total number of elements, then the n = 0 case is [], but when n = 1 do you get [xi], [xf], [(xi+xf) / 2] or something else?

@bjornbm
Owner

My nFromTo 0 xi _ = [xi] is also somewhat arbitrary. Perhaps the most sensible is for n to be the number of intermediate steps so that n == 0 gives [xi, xf]. Perhaps not what one would guess (the total number of elements is arguably the most “intuitive”) but with proper documentation I think that's fine.

@dmcclean

I think the n == 0 gives [xi, xf] approach probably does make sense. It certainly seems confusing at first glance, but with good documentation it should be OK.

I had a thought that it would be really nice if this from/to business went into the intervals package where it could work for things other than just quantities. But then I realized that isn't simple because Num is so monolithic.

This one would be really nice, together with the stuff in dimensional-dk-experimental.

nFrom :: (AdditiveGroup a) => Interval a -> Int -> [a] -- or maybe flipped?

I'm not really enthused by any of these name choices. It doesn't seem like this function should be difficult to name, but it is. sample suggests randomness, interpolate suggests only returning a single value, ...

@bjornbm
Owner

distribute (as in distribute values evenly in the interval)?

@dmcclean

(Whoops, the AdditiveGroup thought was half-baked, that isn't enough structure to be able to divide by n. So it would have to be something even more intimidating, like (Fractional s, s ~ Scalar v, VectorSpace v) => Interval v -> s -> [v]. It could lean on lerp from that package.)

Some thoughts from around the internet:

@dmcclean dmcclean closed this
@dmcclean dmcclean deleted the branch
@bjornbm
Owner

I suspect this was closed implicitly by the removal of your branch? I've opened #47 to track this.

@dmcclean

Indeed, sorry about that. I got a little too aggressive trying to cleanup the fact that I had a lot of branches from closed merge requests.

@bjornbm
Owner

No problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 28, 2014
  1. @dmcclean
This page is out of date. Refresh to see the latest.
Showing with 2 additions and 3 deletions.
  1. +2 −3 Numeric/Units/Dimensional/DK.hs
View
5 Numeric/Units/Dimensional/DK.hs
@@ -52,7 +52,6 @@ Clients probably will want to use the NegativeLiterals extension.
{-# LANGUAGE DataKinds #-}
{-# LANGUAGE FlexibleContexts #-}
{-# LANGUAGE FlexibleInstances #-}
-{-# LANGUAGE GeneralizedNewtypeDeriving #-}
{-# LANGUAGE KindSignatures #-}
{-# LANGUAGE ScopedTypeVariables #-}
{-# LANGUAGE TypeFamilies #-}
@@ -87,7 +86,7 @@ module Numeric.Units.Dimensional.DK
where
import Prelude
- ( Show, Eq, Ord, Enum, Num, Fractional, Floating, RealFloat, Functor, fmap
+ ( Show, Eq, Ord, Num, Fractional, Floating, RealFloat, Functor, fmap
, (.), flip, show, (++), undefined, otherwise, (==), String, unwords
, map, null, Integer, Int, ($), zipWith, uncurry
)
@@ -131,7 +130,7 @@ units and quantities it represents have physical dimensions.
-}
newtype Dimensional (v::Variant) (d::Dimension) a
- = Dimensional a deriving (Eq, Ord, Enum)
+ = Dimensional a deriving (Eq, Ord)
{-
The type variable 'a' is the only non-phantom type variable and
Something went wrong with that request. Please try again.