Opened 12 months ago

Last modified 2 months ago

#15622 new feature request

Generalize `E{0,1,2,3,6,9,12}` from `Data.Fixed`

Reported by: rockbmb Owned by: rockbmb
Priority: normal Milestone: 8.6.1
Component: Core Libraries Version: 8.4.3
Keywords: base, Data.Fixed Cc: Bodigrim, Ashley, Yakeley, RyanGlScott
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

Hello;

I'm creating this email to propose a change to Data.Fixed. Full credit for this idea goes to Bhavik Mehta (@b-mehta on GitHub), who implemented it in this PR for exact-pi.

In Data.Fixed there are several E-prefixed datatypes used to represent a certain number of digits of precision in fixed-precision arithmetic. For example, E1 has 1 decimal place, E12 has 12. Each of them, E{0,1,2,3,6,9,12} is hardcoded. If more precision types are to be provided, they have to be hardcoded as well, and all of these types resemble each other. I think there is room for improvement here.

Instead of having

data E0

instance HasResolution E0 where
    resolution _ = 1

and repeating it as many times as there are E datatypes, I propose to add the following type:

{-# LANGUAGE DataKinds      #-}
{-# LANGUAGE KindSignatures #-}

import GHC.TypeLits (Nat, KnownNat, natVal)

data E (n :: Nat)

and then do

instance KnownNat n => HasResolution (E n) where
    resolution _ = 10^natVal (undefined :: E n)

just once, replacing data E0 with type E0 = E 0 (and the same for the rest of them) to continue reexporting these types. E should also be exported.

I've sent an email to the Core Libraries Committee regarding this issue. This is my first contribution to GHC, if I'm doing something incorrectly please tell me.

Change History (32)

comment:1 Changed 12 months ago by rockbmb

Cc: AshleyYakeley added
Owner: set to rockbmb

comment:2 Changed 12 months ago by rockbmb

Cc: Ashley Yakeley added; AshleyYakeley removed

comment:3 Changed 12 months ago by rockbmb

Cc: "Ashley Yakeley" added; Ashley Yakeley removed

comment:4 Changed 12 months ago by rockbmb

I am trying to add "Ashley Yakeley" to the "Cc: " field, but the whitespace is preventing me from doing so. Can someone help me with this?

comment:5 Changed 12 months ago by rockbmb

Cc: RyanGlScott added

comment:6 Changed 12 months ago by Bodigrim

I've sent an email to the Core Libraries Committee regarding this issue.

Are you sure? I cannot find it in https://mail.haskell.org/pipermail/libraries/2018-September/thread.html

comment:7 Changed 12 months ago by RyanGlScott

Indeed, since this proposes to change the existing API in Data.Fixed, I would first send a mail to the libraries mailing list (which Bodigrim has linked to in comment:6) and solicit community feedback. If there is a consensus that this change should be adopted, we can proceed forward with the actual implementation.

comment:8 Changed 12 months ago by Bodigrim

AFAIU the proposed implementation (https://github.com/ghc/ghc/pull/196) is backward compatible and the only visible change is a new exported entity Data.Fixed.E.

comment:9 Changed 12 months ago by RyanGlScott

That is still a breaking change, since it changes E0, E1, etc. from data types to type synonyms. Among other things, this will cause programs that declare instances against these types to stop compiling if they do not enable the FlexibleInstances extension.

That change notwithstanding, I would also be interested to hear the community's feedback on the use of the DataKinds GHC extension in a prominent place in base like Data.Fixed. (DataKinds is already used in other places in base, but they're mostly sectioned off within the GHC.* namespace, where language extension experimentation is more readily tolerated.)

comment:10 Changed 12 months ago by rockbmb

I received this email from Microsoft not long after I sent an email to libraries@…:

Delivery has failed to these recipients or groups:

Haskell Libraries (libraries@…) Your message couldn't be delivered. Despite repeated attempts to contact the recipient's email system it didn't respond. Contact the recipient by some other means (by phone, for example) and ask them to tell their email admin that it appears that their email system isn't accepting connection requests from your email system. Give them the error details shown below. It's likely that the recipient's email admin is the only one who can fix this problem.

I'll try again.

comment:11 Changed 12 months ago by rockbmb

I just tried sending it again, it does not work. I received no confirmation of having sent the message, and I could not see it in September's threads. I've triple-checked my Mailman settings, I don't know what the issue is.

I'll try to solve this so the mail gets sent.

comment:12 Changed 12 months ago by Bodigrim

Among other things, this will cause programs that declare instances against these types to stop compiling if they do not enable the FlexibleInstances extension.

As I noticed in a CLC mail thread(https://mail.haskell.org/pipermail/libraries/2018-September/028975.html), no code from Hackage derives such instances currently.

comment:13 Changed 12 months ago by rockbmb

Since no code from Hackage derives instances against these types currently, does anyone disagree it is safe to assume no code (published on Hackage) will break from this change?

Projects not uploaded to Hackage or that are closed-source and rely on GHC may have issues if they have instances defined for E0/E1..., but I do not think they will be too great. In these cases, would the addition of FlexibleInstances be problematic? I do not think so, but that is my opinion.

comment:14 Changed 12 months ago by Ashley Yakeley

  1. As written, the proposed change eliminates non-base-10 Fixed. Are we sure we want to do that?
  1. If we're considering breaking changes to Data.Fixed, the more urgent need (imo) is to generalise the representation type, which is currently hard-coded as Integer.

Touching both points, Q formats are fixed-point with a given number of fractional lower bits. It would be useful to be able to represent these.

Last edited 12 months ago by Ashley Yakeley (previous) (diff)

comment:15 Changed 12 months ago by Ashley Yakeley

I would also like to see Data.Fixed (and possibly also Data.Ratio and Data.Complex) put in a new core library.

comment:16 in reply to:  14 Changed 11 months ago by rockbmb

Replying to Ashley Yakeley:

  1. As written, the proposed change eliminates non-base-10 Fixed. Are we sure we want to do that?
  1. If we're considering breaking changes to Data.Fixed, the more urgent need (imo) is to generalise the representation type, which is currently hard-coded as Integer.

Touching both points, Q formats are fixed-point with a given number of fractional lower bits. It would be useful to be able to represent these.

I do not understand the point you make in 1., could you please elaborate? Regarding 2., I understand and agree. Would replacing a hardcoded Integer with Integral a => ... be sufficient?

comment:17 Changed 11 months ago by Ashley Yakeley

Never mind, it looks like my first point is not a concern, because you can still create other instances of HasResolution.

comment:18 in reply to:  17 Changed 11 months ago by rockbmb

Replying to Ashley Yakeley:

Never mind, it looks like my first point is not a concern, because you can still create other instances of HasResolution.

If that is the case, I do not mind addressing your second point in the PR I already made.

Regarding Q formats, this sounds like a worthwhile addition, but it should probably be done in a separate PR, with a ticket here to match.

comment:19 Changed 11 months ago by Bodigrim

Actually, we can generalise the current approach to any base:

data E (n :: Nat)

instance KnownNat n => HasResolution (E n) where
    resolution = natVal . Compose

type Milli = E 1000

With regards to the representation type: it does not make much sense to have Int8 with resolution 1000. And there are only two integer types with arbitrary precision available in base. IMHO it is reasonable to stick to Integer, not least because it allows to avoid having any visible breaking change at all.


I would also like to see Data.Fixed (and possibly also Data.Ratio and Data.Complex) put in a new core library.

I believe it could be desired, if there is a demand for a more rapid development of these modules than for the rest of base, but as far as I see this is not the case. Otherwise I do not see any direct benefits of such breaking change.

comment:20 in reply to:  19 Changed 11 months ago by rockbmb

Replying to Bodigrim:

Actually, we can generalise the current approach to any base:

data E (n :: Nat)

instance KnownNat n => HasResolution (E n) where
    resolution = natVal . Compose

type Milli = E 1000

How would the Compose help with numeric bases here?

comment:21 Changed 11 months ago by Bodigrim

Compose itself makes no difference, of course. The instance from your PR is equivalent to resolution = (10 ^) . natVal . Compose. My suggestion is to put resolution = natVal . Compose, which makes us able to express desired accuracy both in decimal (E 1000) and binary (E 1024) digits, as well as in any other base.

comment:22 Changed 11 months ago by Ashley Yakeley

Alternatively, use Nat directly?

newtype Fixed (n :: Nat) = MkFixed Integer

comment:23 Changed 11 months ago by Ashley Yakeley

Regarding representation type, the performance of the time library, as well as ease of serialisation, might be improvable if DiffTime and NominalDiffTime (which wrap Pico) were represented with 128-bit integer type rather than Integer. See <https://github.com/haskell/time/issues/80>. That said, I'm not feeling a lot of pressure for this at the moment.

comment:24 Changed 11 months ago by Ashley Yakeley

If I were starting over, with no compatibility concerns, I would certainly do this:

newtype Fixed a (n :: Nat) = MkFixed a

comment:25 Changed 11 months ago by Ashley Yakeley

Nat is just a wrapper for Integer, isn't it? As opposed to data Nat = Z | S Nat?

comment:26 in reply to:  22 Changed 11 months ago by Bodigrim

Replying to Ashley Yakeley:

Alternatively, use Nat directly?

newtype Fixed (n :: Nat) = MkFixed Integer

Yeah, looks nice. But one should scan Hackage again to make sure that no one derived HasResolution instances for his own types.

Replying to Ashley Yakeley:

Nat is just a wrapper for Integer, isn't it? As opposed to data Nat = Z | S Nat?

Yes.

comment:27 in reply to:  21 Changed 11 months ago by rockbmb

Replying to Bodigrim:

Compose itself makes no difference, of course. The instance from your PR is equivalent to resolution = (10 ^) . natVal . Compose. My suggestion is to put resolution = natVal . Compose, which makes us able to express desired accuracy both in decimal (E 1000) and binary (E 1024) digits, as well as in any other base.

I understand now, I failed to notice that where I wrote type Milli = E 3, you wrote type Milli = E 1000.

comment:28 in reply to:  24 Changed 11 months ago by rockbmb

Replying to Ashley Yakeley:

If I were starting over, with no compatibility concerns, I would certainly do this:

newtype Fixed a (n :: Nat) = MkFixed a

In this case, isn't one of the two false:

  • One of the type parameters is unnecessary (I'd say a)
  • HasResolution is unnecessary

?

comment:29 Changed 7 months ago by rockbmb

Cc: Ashley Yakeley added; "Ashley Yakeley" removed

comment:30 in reply to:  24 Changed 7 months ago by Bodigrim

I believe there are many shiny things which could be done, if we start from the scratch. Unfortunately, we don't. Also, my understanding is that no one has an appetite for visible breaking changes.

Can we have it implemented as proposed in PR or as described in comment:19?

comment:31 Changed 7 months ago by rockbmb

I've pinged a maintainer on the GitHub's PR page, I would also like to either close or move forward with this, either as it currently is or with @Bodigrim's change.

More elaborate changes can always come afterwards.

comment:32 Changed 2 months ago by Marge Bot <ben+marge-bot@…>

In a31b24a5/ghc:

base: Data.Fixed: make HasResolution poly-kinded (#10055, #15622)
Note: See TracTickets for help on using tickets.