Skip to content

[RFC/WIP] unify the concepts of Fixed and UFixed #32

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

Closed
wants to merge 4 commits into from
Closed

[RFC/WIP] unify the concepts of Fixed and UFixed #32

wants to merge 4 commits into from

Conversation

vchuravy
Copy link
Collaborator

After #27 UFixed and Fixed shared a lot of concepts and they do effectively the same thing, but sometimes a function is only implemented one way.

This is the current status of having only one type of FixedPoint and the underlying Type T defines whether it is a Unsigned or Signed type.

I added in comments the second implementation if the implementation is ambiguous and I haven't looked at the tests yet.

Merging those two concepts simplifies the code immensely, but it also highlights the subtle differences that where between Fixed and UFixed.


# constructor for manipulating the representation;
# selected by passing an extra dummy argument
FixedPoint(y::T, _) = new(i)
Copy link
Member

Choose a reason for hiding this comment

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

y->i? (typo)

@timholy
Copy link
Member

timholy commented Oct 16, 2015

I assume you're noting it's still failing, seemingly due to the "not quite 1" issue I raised above.

We might need a different implementation of convert depending on whether T<:Signed.

@timholy
Copy link
Member

timholy commented Oct 19, 2015

Need any help? I'd love to get this finished, tagged, and then be able to tag new versions for Colors/ColorTypes/ColorVectorSpace/Images etc.

@vchuravy
Copy link
Collaborator Author

Any help is appreciated, grad school got a bit in the way :).

I just pushed the last changes I have around, but I think I am still missing some cases.

So one of the issues is that for signed types 1 is not necessarily part of the representation and maybe using typemax and typemin is a better approach to convert FixedPoint to FloatingPoint.

@@ -1,4 +1,4 @@
VERSION >= v"0.4.0-dev+6521" && __precompile__()
# ERSION >= v"0.4.0-dev+6521" && __precompile__()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oops :)

@vchuravy
Copy link
Collaborator Author

I added some comments, where I still have open questions and not necessary good answers. Essentially every bit of functionality that has a commented out version I was a bit unsure about.

@vchuravy
Copy link
Collaborator Author

A fundamental problem I am running into that using rawone doesn't make sense for Fixed... and it is a bit unintuitive that 0xFF equals 1.0 for UFixed8 = FixedPoint{UInt8, 8} where f = 8 is the number of fractional bits, with setting 0xff we are using a fractional bit to signal oneness.

Using FixedPoint{UInt8, 7} Where the first bit is the integral bit would break the major use of unsigned fixedpoint numbers in colors...

A similar problem arises with signed fixedpoint numbers, where we need one bit to signal sign and so the question is if FixedPoint{Int8, 8} is well defined at all.

Maybe my brain is just going off on a tangent and I don't see the forest because of all the trees...

@timholy
Copy link
Member

timholy commented Oct 23, 2015

You know, this rings a bell and is probably why I didn't unify them when I first wrote Ufixed. It has been so long, I had forgotten about these considerations. For colors/images we definitely need 0xff to correspond to 1, so perhaps these are just two different beasts and should remain separate.

Sound reasonable?

@vchuravy
Copy link
Collaborator Author

I am going to think this over, getting rid of a lot of code duplication would be really nice, but I don't really want to break the one major use case of FixedPointNumbers. (Even though UFixed should probably be called normalized numbers ;) )

@timholy
Copy link
Member

timholy commented Oct 23, 2015

Sounds good. I'll be out of touch for a couple of days.

When I get back, I'll soon go ahead and tag a new release of Images and all related packages, just because it's overdue. If it takes a little longer to figure this out, that's fine---no rush.

@vchuravy
Copy link
Collaborator Author

I am still interested in renaming UFixed to UNormed or UNorm. But I don't think anymore that having Fixed{Unsigned} is really useful.

@vchuravy vchuravy closed this Sep 14, 2016
@timholy
Copy link
Member

timholy commented Sep 16, 2016

I could go with such a renaming. (I probably won't do it myself, but happy to see it arrive, and even happier if someone helps fix the deprecations it will cause elsewhere 😉---EDIT: but don't worry about Images, that's in too much flux right now for anyone but me to touch.)

@timholy
Copy link
Member

timholy commented Sep 17, 2016

If we're thinking about names, some other possible changes to consider:

  • rename FixedPoint to AbstractFixedPoint
  • rename Fixed to FixedPoint (requires the above, and should probably be done in separate minor releases with suitable deprecation warnings and some reasonable time delay...)
  • as alternatives to Normed, also consider UnitFixedPoint (with unit in the sense of a unit vector), NormalizedFixedPoint, FixedPointOne, FixedOne, FixedZO (Z=zero, O=one).

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