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

Initialise alt with type specific 0 instead of Float64 #28

Merged
merged 1 commit into from
Feb 19, 2017

Conversation

lwabeke
Copy link

@lwabeke lwabeke commented Jan 20, 2017

Without this change constructors using Float32 variables fail.

Without this change constructors using Float32 variables fail.
@lwabeke
Copy link
Author

lwabeke commented Jan 20, 2017

The keyword constructor is still a problem:
LLA(;lat=NaN,lon=NaN,alt=0.0)
But not sure how to fix this.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.09%) to 77.984% when pulling e6276a1 on lwabeke:bugfix-on-constructor into 378259b on JuliaGeo:master.

@lwabeke
Copy link
Author

lwabeke commented Jan 20, 2017

Quick attempt to try to address
#29

@andyferris
Copy link
Contributor

Thanks @lwabeke - looks good.

Keyword constructors are currently not type-stable (meaning, in general in Julia, unless they have a fully-defined type like alt::Float64 = 0.0, but this of course doesn't fix your problem) so I'm not worried about this. Have you tried alt = zero(lon), as default values can usually depend on earlier ones (but I'm not as certain about keyword arguments).

@lwabeke
Copy link
Author

lwabeke commented Jan 23, 2017

The problem in that keyword constructor is that NaN if already Float64, thus unless all 3 are specified as being a different type, it will cause problems. But I'm also not too worried about that case.

@c42f c42f merged commit 526e44c into JuliaGeo:master Feb 19, 2017
@c42f
Copy link
Member

c42f commented Feb 19, 2017

Eek, we should have merged this ages ago, thanks for the patch.

@omus omus mentioned this pull request Jun 20, 2017
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