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 tofloat(Point) #1604

Merged
merged 1 commit into from
Feb 2, 2024
Merged

Add tofloat(Point) #1604

merged 1 commit into from
Feb 2, 2024

Conversation

Sbozzolo
Copy link
Member

@Sbozzolo Sbozzolo commented Feb 1, 2024

It is easy to go from a number to a Geometry.Point, but it is not easy to go in the other direction (unless the type of point of point is known). This PR adds new methods to Float32, Float64, and BigFloat to X/Y/Z/Lat/Long point to convert them into numbers.

This feature would be very useful in converting a Field of ZPoints into a scalar field, which is what we need for the interpolation routines

@Sbozzolo Sbozzolo force-pushed the gb/points_to_floats branch 2 times, most recently from 475f41f to fb741f5 Compare February 1, 2024 22:30
Copy link
Member

@dennisYatunin dennisYatunin left a comment

Choose a reason for hiding this comment

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

Overloading the constructors for primitive types is a bit risky. Is there a downside to defining a function that does this, like Geometry.first_coordinate(::Point)?

@Sbozzolo
Copy link
Member Author

Sbozzolo commented Feb 1, 2024

Overloading the constructors for primitive types is a bit risky. Is there a downside to defining a function that does this, like Geometry.first_coordinate(::Point)?

This function also converts to the specified float type. This is not strictly needed, but it is nice to have. Geometry.coordinate already exists but doesn't return a float, it returns a Point, so maybe I could define tofloat

@dennisYatunin
Copy link
Member

dennisYatunin commented Feb 1, 2024

Overloading the constructors for primitive types is a bit risky. Is there a downside to defining a function that does this, like Geometry.first_coordinate(::Point)?

This function also converts to the specified float type. This is not strictly needed, but it is nice to have. Geometry.coordinate already exists but doesn't return a float, it returns a Point, so maybe I could define tofloat

Yes, a more "standard" way to implement this would be something like Geometry.tofloat(::Type{T}, ::Point), with a default value for T based on the Point type. Adding constructors to the float types implies that our points are "equivalent" to floats at a low level, which is not quite the case.

@Sbozzolo Sbozzolo force-pushed the gb/points_to_floats branch from fb741f5 to cc70338 Compare February 1, 2024 23:44
@Sbozzolo
Copy link
Member Author

Sbozzolo commented Feb 1, 2024

Overloading the constructors for primitive types is a bit risky. Is there a downside to defining a function that does this, like Geometry.first_coordinate(::Point)?

This function also converts to the specified float type. This is not strictly needed, but it is nice to have. Geometry.coordinate already exists but doesn't return a float, it returns a Point, so maybe I could define tofloat

Yes, a more "standard" way to implement this would be something like Geometry.tofloat(::Type{T}, ::Point), with a default value for T based on the Point type. Adding constructors to the float types implies that our points are "equivalent" to floats at a low level, which is not quite the case.

I added a function tofloat. It returns the same type as the Point, which is fine. If we need more, we can add it.

Copy link
Member

@charleskawczynski charleskawczynski left a comment

Choose a reason for hiding this comment

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

With the two changes I suggested, I'm fine with this PR.

@Sbozzolo Sbozzolo force-pushed the gb/points_to_floats branch from cc70338 to 12b1ebc Compare February 2, 2024 16:02
It is easy to go from a number to a Geometry.Point, but it is not easy
to go in the other direction (unless the type of point of point is
known). This PR adds a new function `tofloat` to X/Y/Z/Lat/Long point to
convert them into numbers.

This feature would be very useful in converting a Field of ZPoints into
a scalar field, which is what we need for the interpolation routines
@Sbozzolo Sbozzolo force-pushed the gb/points_to_floats branch from 12b1ebc to ee27f1a Compare February 2, 2024 16:30
@Sbozzolo Sbozzolo changed the title Add FT(Point) Add to_float(Point) Feb 2, 2024
@Sbozzolo Sbozzolo changed the title Add to_float(Point) Add tofloat(Point) Feb 2, 2024
@charleskawczynski charleskawczynski merged commit 78ab9a8 into main Feb 2, 2024
16 checks passed
@charleskawczynski charleskawczynski deleted the gb/points_to_floats branch February 2, 2024 18:34
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.

3 participants