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

Implement Radian and Degree #47

Merged
merged 5 commits into from
Feb 4, 2015
Merged

Implement Radian and Degree #47

merged 5 commits into from
Feb 4, 2015

Conversation

mweastwood
Copy link
Contributor

I need radians for my work, so I went ahead and took the first steps towards implementing them. I think the utility of having an angular unit outweighs the fact that radians are not technically an offical "SI unit". Also, I'm not hugely familiar with SIUnits, so there are still a couple questions I've left open.

  • Should I define the trigonometric functions for quantities whose dimensions are purely angular?
  • Should degrees be implemented as a non-SI unit?
  • Do radians even belong in an SIUnits package?

Finally, I should probably add some new tests, but I'm tired, so that will wait for tomorrow.

@mweastwood
Copy link
Contributor Author

I updated this pull request by defining Degree as a NonSIUnit, defining the usual trig functions for both Degrees and Radians, as well as adding some minimal tests.

@mweastwood mweastwood changed the title [RFC] Implementing radians Implement Radian and Degree Jan 3, 2015
Additionally define the trig functions to automatically call their
degree-based variant (eg. make cos(1.0Degree) call cosd(1.0))
@mweastwood
Copy link
Contributor Author

Bump & rebased.

Radians and steradians are the only two named dimensionless derived SI units. Therefere this does not open a bottomless pit of units we could potentially add.

@tomasaschan
Copy link
Collaborator

I haven't had time to examine the code in detail, but conceptually I like the idea of introducing angular units.

However, we do have to consider if we want to allow things like 1 + 2 rad - I think it would be silly, but it is technically valid. (Currently this PR doesn't allow this, I presume?)

This is also a breaking change for any code that interfaces with the (unexported) SIQuantity type directly - might be more than a few of those out there, especially with things like unitful arrays - so I think we should tag a release before this is merged, so people can pin the package if they need to.

@tomasaschan
Copy link
Collaborator

Also, does it make sense to define steradians as a derived unit, defined as radians squared, i.e. sr = rad^2 = SIUnit{0,0,0,0,0,0,0,2}?

@mbauman
Copy link
Collaborator

mbauman commented Jan 7, 2015

This is also a breaking change for any code that interfaces with the (unexported) SIQuantity type directly

… or anyone who has saved JLD or serialization files with SIQuantities, which is most certainly more common.

julia> using HDF5, JLD

julia> type Foo{a}; end

julia> x = Foo{1}()
Foo{1}()

julia> @save "test.jld" x

julia> open("test.jls","w") do f; serialize(f, x); end

julia> load("test.jld")
Dict{Union(UTF8String,ASCIIString),Any} with 1 entry:
  "x" => Foo{1}()

julia> open("test.jls") do f; deserialize(f); end
Foo{1}()

$ julia --quiet
julia> using HDF5, JLD

julia> type Foo{a,b}; end

julia> load("test.jld")
ERROR: unexpected non-leaf type Foo{1,b}
 in h5type at /Users/mbauman/.julia/v0.3/HDF5/src/jld_types.jl:358
 in jldatatype at /Users/mbauman/.julia/v0.3/HDF5/src/jld_types.jl:636
 in read at /Users/mbauman/.julia/v0.3/HDF5/src/JLD.jl:328
 in read at /Users/mbauman/.julia/v0.3/HDF5/src/JLD.jl:313
 in anonymous at /Users/mbauman/.julia/v0.3/HDF5/src/JLD.jl:966
 in jldopen at /Users/mbauman/.julia/v0.3/HDF5/src/JLD.jl:234
 in load at /Users/mbauman/.julia/v0.3/HDF5/src/JLD.jl:965

julia> open("test.jls") do f; deserialize(f); end
Foo{1,b}()

@tomasaschan
Copy link
Collaborator

Yeah, that too. But I'm not sure interfacing with SIQuantity is so uncommon, given that it's required to get fast unitful arrays (until we fix #25) ;)

Anyway, to be clear I don't think this being a breaking change is an argument against having this functionality in the package; I just wanted to make sure we're concious about tagging versions etc when merging this, to make it as easy as possible for people who don't want this change (yet).

@mweastwood
Copy link
Contributor Author

I think defining sr = rad^2 is misleading because this only works in the small angle approximation. Example:

  • For small angles, the solid angle subtended by a cone (with opening angle 2theta) is solid angle = pi * (theta)^2.
  • For larger angles, solid angle = 2pi * (1-cos(theta)).

Correctly introducing solid angles will require another breaking change (because you need another type parameter). Therefore if steradians are something we want, it might make sense to add them with this PR as well.

@mweastwood
Copy link
Contributor Author

So what's the deal here? Does this need to come bundled with other breaking changes? (#32, #40)

@tomasaschan
Copy link
Collaborator

@mweastwood I do think it would be nice to add solid angles as well, and that it should be done in this PR, but I don't think it's necessary to bundle this with either #32 or #40. Tagging a version in between this change and those shouldn't be a problem.

@Keno
Copy link
Owner

Keno commented Jan 13, 2015

Sorry, for not responding earlier. I'll bump looking over this on my todo list and get back to you as soon as I can.

@mweastwood
Copy link
Contributor Author

I've now added steradians to this PR

@tomasaschan
Copy link
Collaborator

I think you forgot to export Steradian, but otherwise this LGTM.

As noted above, though, we should make sure to tag a version right before this is merged.

@mweastwood
Copy link
Contributor Author

@tlycken Although that's the kind of mistake I usually make, I believe I did export Steradian here

@tomasaschan
Copy link
Collaborator

@mweastwood Yes, sorry - I was a bit too quick there. Looking back I don't know why I missed it; probably I looked at the export of Degree in nonsiunits.jl and got confused when there (correctly) wasn't a corresponding unit for solid angles. All well, then! =)

@mweastwood
Copy link
Contributor Author

@Keno Just keeping this on your radar.

@mweastwood
Copy link
Contributor Author

@Keno Can you look this over?

@Keno
Copy link
Owner

Keno commented Feb 3, 2015

Yes, sorry, will do right away.

@Keno
Copy link
Owner

Keno commented Feb 3, 2015

Couldn't steradian just be radian squared, why does it need a separate space to keep track of it?

@mweastwood
Copy link
Contributor Author

The issue is that for a given angle theta (in radians), the solid angle subtended by a cone with opening angle 2 * theta is pi * theta^2 only in the small angle limit. This approximation breaks down at large angles. Hence equating steradians and radians squared is a little misleading. There's a little discussion about this above.

@mweastwood
Copy link
Contributor Author

However, I'd be willing to accept a decision either way because sr = rad^2 works reasonably well for a lot of use cases.

@Keno
Copy link
Owner

Keno commented Feb 4, 2015

I'll merge this for now but might refactor as part of a larger SIUnits refactor later.

Keno added a commit that referenced this pull request Feb 4, 2015
Implement `Radian` and `Degree`
@Keno Keno merged commit 6e840cf into Keno:master Feb 4, 2015
@Keno
Copy link
Owner

Keno commented Feb 4, 2015

Thanks!

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