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

Remove DualQuaternion #92

Merged
merged 22 commits into from
Oct 6, 2022
Merged

Remove DualQuaternion #92

merged 22 commits into from
Oct 6, 2022

Conversation

sethaxen
Copy link
Collaborator

This PR removes DualQuaternion and the dependency on DualNumbers. It instead adds an example to the documentation of using ForwardDiff with Quaternions to get the same functionality. The code in that example is actually the first steps one could take to define a package with just such utilities for dual quaternions using these 2 packages.

This is a breaking change, but we may want to make some more breaking changes before releasing, so the version number is marked -DEV.

@codecov
Copy link

codecov bot commented Sep 16, 2022

Codecov Report

Merging #92 (620087f) into master (b1f50c9) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff             @@
##            master       #92    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files            3         2     -1     
  Lines          391       287   -104     
==========================================
- Hits           391       287   -104     

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@hyrodium
Copy link
Collaborator

I think it would be better to release v0.5.7 with Base.depwarn (or Base.@deprecate). The next breaking release with v0.6.0 includes the following breaking changes, so it would be helpful to add detailed deprecated messages.

  • Remove linpol (This function is already deprecated with Base.@deprecate)
  • Remove imag (This function is already deprecated with Base.@deprecate)
  • Remove DualQuaternion (I'm now making a PR to add deprecated messages)
  • Remove cis/cispi Remove cis/cispi #76 (This doesn't have deprecated messages.)

I'm not sure the following breaking changes will be included in v0.6.0.

Copy link
Collaborator

@hyrodium hyrodium left a comment

Choose a reason for hiding this comment

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

Great! Thanks for the PR. I have added minor suggestions.

docs/src/examples/dual_quaternions.md Outdated Show resolved Hide resolved
docs/src/examples/dual_quaternions.md Outdated Show resolved Hide resolved
docs/src/examples/dual_quaternions.md Show resolved Hide resolved
docs/src/examples/dual_quaternions.md Outdated Show resolved Hide resolved
@sethaxen
Copy link
Collaborator Author

I think it would be better to release v0.5.7 with Base.depwarn (or Base.@deprecate). The next breaking release with v0.6.0 includes the following breaking changes, so it would be helpful to add detailed deprecated messages.

* Remove `linpol` (This function is already deprecated with `Base.@deprecate`)

* Remove `imag` (This function is already deprecated with `Base.@deprecate`)

* Remove `DualQuaternion` (I'm now making a PR to add deprecated messages)

* Remove `cis/cispi` [Remove cis/cispi #76](https://github.com/JuliaGeometry/Quaternions.jl/pull/76) (This doesn't have deprecated messages.)

Good idea!

I'm not sure the following breaking changes will be included in v0.6.0.

* Remove `Octonion` (This should be done after creating Octonions.jl package, maybe in JuliaGeometry?)

I'll comment on this in #90.

* Remove rotation-related functions [Drop inefficient functions for rotation? #86](https://github.com/JuliaGeometry/Quaternions.jl/issues/86)

This seems fine to be in a later breaking release.

* Remove `Complex`-`Quaternion` compatibility [Should `Complex` be convertible to `Quaternion`? #62](https://github.com/JuliaGeometry/Quaternions.jl/issues/62)

This will be a little easier to do once this package contains just Quaternion, although it wouldn't be too much of a burden to do in v0.6.0. Why do you think it should wait?

* Change `norm` field to property [Add isunit and change norm field to property #75](https://github.com/JuliaGeometry/Quaternions.jl/pull/75) (`q.norm == true` does not lead `(q^2).norm == true` after this PR. It seems better to regard this PR as breaking changes, just in case.)

Yeah, dropping this is a priority, but it's a lot easier after Quaternion and DualQuaternion are gone. It could be in v0.6.0, but that already seems busy, so it can wait.

@sethaxen sethaxen mentioned this pull request Sep 19, 2022
@hyrodium
Copy link
Collaborator

This will be a little easier to do once this package contains just Quaternion, although it wouldn't be too much of a burden to do in v0.6.0. Why do you think it should wait?

I was just concerned about the release of v0.6.0 will be delayed. It's before v1.0.0, so we can split the breaking changes.

Yeah, dropping this is a priority, but it's a lot easier after Quaternion and DualQuaternion are gone. It could be in v0.6.0, but that already seems busy, so it can wait.

I agree with that. (Quaternion is a typo and should be Octonion? 😄)

This was referenced Sep 20, 2022
@hyrodium
Copy link
Collaborator

The description should be updated, but I don't have permission for that.

image

@sethaxen
Copy link
Collaborator Author

Good point, I've reached out to request I be upgraded to admin.

@sethaxen
Copy link
Collaborator Author

sethaxen commented Oct 2, 2022

This PR should be ready for final review.

Copy link
Collaborator

@hyrodium hyrodium left a comment

Choose a reason for hiding this comment

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

Most of the example seems fine! Just added some suggestions.

docs/src/examples/dual_quaternions.md Outdated Show resolved Hide resolved
docs/src/examples/dual_quaternions.md Outdated Show resolved Hide resolved
docs/src/examples/dual_quaternions.md Outdated Show resolved Hide resolved
docs/src/examples/dual_quaternions.md Show resolved Hide resolved
Copy link
Collaborator

@hyrodium hyrodium left a comment

Choose a reason for hiding this comment

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

LGTM!

@sethaxen
Copy link
Collaborator Author

sethaxen commented Oct 5, 2022

Thanks for the suggestions! I made a few more changes. Should be finished now.

Copy link
Collaborator

@hyrodium hyrodium left a comment

Choose a reason for hiding this comment

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

Thanks for the updates!

@sethaxen sethaxen merged commit 5758dad into master Oct 6, 2022
@sethaxen sethaxen deleted the rmdual branch October 6, 2022 06:09
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.

2 participants