Skip to content

Conversation

@pthariensflame
Copy link

@pthariensflame pthariensflame commented Jan 29, 2014

  • Various improvements to Interval:
    • Added ival' and ispan.
    • Corrected Show instance to always produce valid Haskell code.
    • Added valid Read instance in correspondence with the new Show instance.
    • Corrected Eq instance so that it behaves mathematically sensibly.
    • Replaced broken Ord instance with an instance for PartialOrd.
    • Added logfloat as a dependency to support this.
    • Re-exported PartialOrd and comparingPO to make this easy to use.
    • Added Bounded instance.
    • Added IntervalNesting newtype, supporting an alternative PartialOrd instance around interval nesting.
  • Implemented encodeFloat and decodeFloat (such as they are) for Dif.

- made the constructor `I` strict for better performance
- added `ival'` and `ispan`
- corrected `Show` instance to always produce valid Haskell code
- added valid `Read` instance in correspondence with the new `Show` instance
- added `Bounded` instance
such as they are, anyway
Thanks, Travis.
Especially when currying is involved
Hopefully that's it!
@DanBurton
Copy link
Collaborator

Thanks for the pull request! I'm going to put my critical cap on, challenge a few of these design decisions, and ask some more people to review these changes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this strictness is justified. Theoretically there might be lazy numeric types that we might want to use this with. And anyways, if the user is invoking ival or ival' to construct an Interval, then the code will perform <=, which in most cases will force the numbers at that point so they can be compared.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can appreciate that you might want to support lazy types, but in that case all the other types in the library should be changed to reflect that. I just noted that everything except Interval is currently strict, and assumed that you wanted strictness to be the default. If that assumption was wrong, I can change all the other types instead. :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really have an opinion one way or the other; I just want to make sure I don't screw up numbers for anybody who might be using it. Lazifying the rest of the library might be the right way to go. Let's try and connect with a few other numbers users and see what they think. I just noticed in this particular case that there doesn't seem to be any benefit to making these fields strict other than source code consistency.

@DanBurton
Copy link
Collaborator

I've started a discussion on Reddit.

http://www.reddit.com/r/haskell/comments/1whu8s/request_for_review_numbers/

In preperation for rewriting the `Ord` instance of `Interval`
Also makes the `Eq` instance behave somewhat more sensibly.
Additionally, this removes the internal strictness of `Interval`.
This allows an alternative `PartialOrd` instance to exist.
Also re-exports `PartialOrd` and `comparingPO` from this module.
Where did you go, `where`?
@cartazio
Copy link
Owner

i'd advocate log-domain rather than log-float if you really wanted to add log domain floats. (which i don't really see being used, its just picked up because it has a partial-ord class, which i'd be inclined to be against. Unless you're saying its the canonical choice of hypothetical partial ord class)

likewise, i'm not sure if you gain much by adding a partialOrd type class, a simple partialCompare function for intervals would be just fine!

removing the "slightly illegal" instances, like Ord, while "valid", we have to accept that base has "slightly illegal" instances too,

@cartazio
Copy link
Owner

in case its not clear, I really don't like log-float, i like log-domain is a better lib :)

@cartazio
Copy link
Owner

just to be clear: i do favor improving numbers, but if we're going to deal with illegal instances, we should work on a whole new number prelude! Working around the flaws in the stand number prelude in a localized way just won't buy you that much.

@pthariensflame
Copy link
Author

I don't think just a partialCompare function would cut it. I think a full PartialOrd type class is absolutely necessary; I don't really care where it comes from, but I would rather use an existing one than add yet another copy to numbers. If you can find a package other than logfloat that implements a full PartialOrd type class, I would be happy to take a look at it.

@DanBurton
Copy link
Collaborator

@cartazio, regarding "we should work on a whole new number prelude!", that's been done numerous times (pun intended), and I don't think that's really what this library is for. As I understand it, the main purpose for this library is for use with lambdabot, to facilitate quick-n-dirty numeric stuff on top of the functionality Prelude provides. It may be true that "working around the flaws in the stand number prelude in a localized way just won't buy you that much," but that's pretty much what this library does, and I'm not inclined to radically alter it.

@cartazio
Copy link
Owner

@DanBurton agreed :)

@DanBurton
Copy link
Collaborator

@pthariensflame upon reflection, I don't like adding a dependency and pulling in the PartialOrdering class. It's obscure, and I don't think anybody will use it. (Do you have a use case for using this instance?)

So I'm inclined to mostly reject this pull request. There are, however, a couple things I'd like to bring in:

I like adding encodeFloat and decodeFloat for Dif. I think we should instead implement decodeFloat in a way consistent with the rest of the code:

decodeFloat = decodeFloat . val

It gives an answer of questionable meaningfulness for derivitives, but this is no different from a lot of those other instance methods.

I like the show/read changes.

I like ispan, but not ival'.

I like the Eq change, I think interval identity should be defined as equality of the endpoints of the interval, and anything else should be "not equal".

I don't like the current Ord instance, but I think it should stay. It just needs to be documented.

@pthariensflame
Copy link
Author

@DanBurton Alright; feel free to just copy what you want from this and leave the reas. :)

@DanBurton
Copy link
Collaborator

I'm transferring ownership of the repository to to @jwiegley, so I'll just leave this PR open and let him decide how to handle it.

@jwiegley
Copy link
Collaborator

jwiegley commented Jan 6, 2017

I've been too far removed from this project for too long, and I'm ready to give this up to the next maintainer, who can make a decision on this issue. @cartazio Are you up for that? Or @DanBurton do you want to take it back? I didn't end up using it as much as I had intended, and now my focus is elsewhere, and not on Haskell numerics.

@cartazio
Copy link
Owner

cartazio commented Jan 7, 2017 via email

@jwiegley
Copy link
Collaborator

jwiegley commented Aug 9, 2022

Is there anyone who would like to maintain this package, and to have this GitHub repository transferred over to them?

@cartazio
Copy link
Owner

cartazio commented Aug 9, 2022

i thought i had done a bunch a few years ago, let me double check if i have that branch somewhere just in case i didn't release it

@cartazio
Copy link
Owner

cartazio commented Aug 9, 2022

yup, looks like I had a bunch started, happy to take over or if theres someone who's keen to do a more energetic maintainership, that'd be great too

@jwiegley
Copy link
Collaborator

jwiegley commented Aug 9, 2022

@cartazio Great, if you can drop your cartazio/numbers repository I'll do the transfer!

@cartazio
Copy link
Owner

cartazio commented Aug 9, 2022

i'm a little headless chicken the next few days, i'll make sure i have a calendar reminder to do it this weekend :)

@cartazio
Copy link
Owner

I'll email/message you or this thread in the next week or so once i've done that, juggling some personal matters this month

@jwiegley
Copy link
Collaborator

@cartazio OK, just to add a little fire, I will close this PR next month if no further activity occurs. :) Trying to get my account to PR inbox zero.

@cartazio
Copy link
Owner

rename done :)

@jwiegley
Copy link
Collaborator

Thanks! But GitHub is saying:

cartazio already has a repository in the jwiegley/numbers network

@cartazio
Copy link
Owner

oh, i'll fix that

@cartazio
Copy link
Owner

fixed!

@jwiegley
Copy link
Collaborator

@cartazio You should have the invite to transfer now...

@cartazio
Copy link
Owner

reviewing this PR now, sorry for the terrible latency

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.

4 participants