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

Add isunit and change norm field to property #75

Closed
wants to merge 10 commits into from

Conversation

sethaxen
Copy link
Collaborator

As suggested in #60 (comment), this PR adds an exported function isunit for checking if a number is normalized. It also changes the norm field to a (deprecated) property, which just forwards to isunit. All constructors and shorthands that use take a boolean argument for whether the number is unit are also deprecated.

This is not a breaking change. However, it should wait until #74 is merged so that we are sure it doesn't break existing functionality.

This will close #60


Base.@irrational INV_SQRT_EIGHT 0.3535533905932737622004 sqrt(big(0.125))

"""
isunit(x)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This naming is most intuitive (we say a "unit quaternion"), but issign would be more consistent with the definition that x == sign(x).

@mikmoore
Copy link
Contributor

mikmoore commented Feb 26, 2022

I made a comment on #60 about the default tolerance for issign (or isunit or whatever we call it). Depending on where we use issign, I'd have different thoughts. If we leave shortcuts in inv, normalize, and other functions based on it, the tolerance should be quite strict (maybe 10eps(T)). If we don't use it internally but just have it for user convenience, we can make it a bit looser. In either case, we should have an optional argument to specify the tolerance.

Of course, I'd certainly support keeping this PR limited to a simple replacement of .isnorm and deferring changes to shortcuts to a separate PR.

@hyrodium hyrodium mentioned this pull request Mar 4, 2022
2 tasks
@sethaxen sethaxen mentioned this pull request Mar 6, 2022
@hyrodium
Copy link
Collaborator

I think we were planning to release breaking releases twice.

  • Change norm field to property (q1.norm&q2.norm does not imply (q1*q2).norm with this change.)
  • Remove norm property (q.norm doesn't work with this change.)

How about combining these changes in one breaking release?
I thought it will be okay with adding a warning to getproperty(::Quaternion, :norm), and then remove it.

@sethaxen
Copy link
Collaborator Author

How about combining these changes in one breaking release?

Yeah, makes sense to me.

I thought it will be okay with adding a warning to getproperty(::Quaternion, :norm), and then remove it.

If we remove q.norm, then I don't think we should raise a warning in this case. It's like keeping a deprecation warning after removing a deprecated function.

This PR is very stale, and I'll open a new one.

@hyrodium
Copy link
Collaborator

hyrodium commented Dec 1, 2022

#108 will be merged soon, so I'll close this PR.

@hyrodium hyrodium closed this Dec 1, 2022
@sethaxen sethaxen deleted the isunit branch December 1, 2022 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do we really need normalized-flag in Quaternion?
3 participants