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

Removed the Enum instance for Dimensional. #27

Closed
wants to merge 1 commit into from

Conversation

dmcclean
Copy link
Collaborator

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
Copy link
Owner

bjornbm commented Feb 28, 2014

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
Copy link
Collaborator Author

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
Copy link
Owner

bjornbm commented Feb 28, 2014

*~~ 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
Copy link
Collaborator Author

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

@bjornbm
Copy link
Owner

bjornbm commented Feb 28, 2014

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

@dmcclean
Copy link
Collaborator Author

I just checked and it doesn't in 7.6.

@dmcclean
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Owner

bjornbm commented Mar 3, 2014

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

@dmcclean
Copy link
Collaborator Author

dmcclean commented Mar 3, 2014

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
Copy link
Owner

bjornbm commented Mar 4, 2014

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
Copy link
Collaborator Author

dmcclean commented Mar 4, 2014

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
Copy link
Owner

bjornbm commented Mar 5, 2014

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
Copy link
Collaborator Author

dmcclean commented Mar 5, 2014

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
Copy link
Owner

bjornbm commented Mar 5, 2014

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
Copy link
Collaborator Author

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
Copy link
Owner

bjornbm commented Jul 17, 2014

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

@dmcclean
Copy link
Collaborator Author

(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:

@bjornbm
Copy link
Owner

bjornbm commented Sep 19, 2014

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

@dmcclean
Copy link
Collaborator Author

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
Copy link
Owner

bjornbm commented Sep 19, 2014

No problem.

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.

2 participants