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

Fix imrotate for Irrational θ #149

Merged
merged 4 commits into from
Oct 11, 2021
Merged

Fix imrotate for Irrational θ #149

merged 4 commits into from
Oct 11, 2021

Conversation

timholy
Copy link
Member

@timholy timholy commented Oct 10, 2021

I was just about to release 0.9.1 (in the wake of #148) and happened to try the test cases in #79, and got this:

julia> mod2pi(π)
ERROR: MethodError: no method matching rem2pi(::Irrational{:π}, ::RoundingMode{:Down})
Closest candidates are:
  rem2pi(::Float64, ::RoundingMode{:Down}) at math.jl:1039
  rem2pi(::Float32, ::RoundingMode) at math.jl:1098
  rem2pi(::Float16, ::RoundingMode) at math.jl:1099
  ...
Stacktrace:
 [1] mod2pi(x::Irrational{:π})
   @ Base.Math ./math.jl:1128

That would not have been a nice thing to release. I'm glad luck worked on my side this time 😅

@codecov
Copy link

codecov bot commented Oct 10, 2021

Codecov Report

Merging #149 (0e3e44d) into master (516dbf6) will increase coverage by 0.23%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #149      +/-   ##
==========================================
+ Coverage   89.90%   90.14%   +0.23%     
==========================================
  Files           8        8              
  Lines         208      213       +5     
==========================================
+ Hits          187      192       +5     
  Misses         21       21              
Impacted Files Coverage Δ
src/warp.jl 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 516dbf6...0e3e44d. Read the comment docs.

@timholy
Copy link
Member Author

timholy commented Oct 10, 2021

I guess one other thing to think about: is the change in size of the returned images breaking? Should this be 0.10 instead?

@timholy
Copy link
Member Author

timholy commented Oct 10, 2021

Might be best to find out whether JuliaGeometry/Rotations.jl#163 meets with approval, and if so just require it.

Copy link
Member

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

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

Unsure how it implies, but it looks good to me.

@timholy
Copy link
Member Author

timholy commented Oct 11, 2021

Unsure how it implies

Sorry, I'm not sure I know what you mean. Do you mean, you're not sure how that Rotations PR applies? We wouldn't need rotmtrx2 anymore. But given that master is a bit broken, perhaps it's best to go with this for now and rework this as needed later.

@timholy timholy merged commit 78adbd0 into master Oct 11, 2021
@timholy timholy deleted the teh/fix_irrational branch October 11, 2021 11:43
@timholy
Copy link
Member Author

timholy commented Oct 11, 2021

Any thoughts on the 0.9.1 vs 0.10 issue? That was the main thing I pinged you for (sorry, I know that wasn't clear).

@johnnychen94
Copy link
Member

johnnychen94 commented Oct 11, 2021

Do you mean, you're not sure how that Rotations PR applies?

Sorry for the inaccuracy, I meant I wasn't very sure how this PR co-works with JuliaLang/julia#42595 and JuliaGeometry/Rotations.jl#163. IIUC JuliaLang/julia#42595 doesn't improve this PR, am I right?

Any thoughts on the 0.9.1 vs 0.10 issue?

Ah, I missed the first comment. I think we can make it 0.9.1 here as it's a clear improvement from my point of view, can you also update the CHANGELOG.md? Thanks again for taking care of this!

@timholy
Copy link
Member Author

timholy commented Oct 11, 2021

Ah, gotcha. Yes, we might not even need to change Rotations if the Base PR goes through. I could put definitions into Compat and then they would be available for previous Julia versions.

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