Skip to content
This repository has been archived by the owner on Nov 22, 2023. It is now read-only.

Add width getter function for HyperCube #195

Merged
merged 1 commit into from
Feb 24, 2020

Conversation

mforets
Copy link
Contributor

@mforets mforets commented Feb 16, 2020

No description provided.

@SimonDanisch
Copy link
Member

That seems to be exactly the same definition as widths?

@SimonDanisch
Copy link
Member

SimonDanisch commented Feb 16, 2020

Ah, sorry.. I forgot that HyperCube does actually store it's width as a single float...
Hm, i'm still not sure if I like that definition :D Seems a bit ambigious - maybe this doesn't need to be a function, and if one has a hypercube, one should just access the field?
We usually only need to have this defined as a function, if we plan to follow an interface that spans multiple types... But I don't really see how the definition of width can easily span over other multi dimensional geometry types ;)

@mforets
Copy link
Contributor Author

mforets commented Feb 16, 2020

one should just access the field?

If you decide to change the internal representation, or field names of the HyperCube in the future, downstream code accessing the field directly will break. If a getter function is provided, this is less likely. That was the motivation for this PR :) I can do widths(X)[1] but that creates an unnecessary 3d vector.

@sjkelly
Copy link
Member

sjkelly commented Feb 17, 2020

I think this is okay since widths gives a vector and width a scalar. You could feasibly define width for other equally-sized geometries such as HyperSpheres and n-gons.

@SimonDanisch
Copy link
Member

Well, I guess we can add this ;)

@SimonDanisch
Copy link
Member

Maybe not export it, since it seems like a widely used function^^

@mforets
Copy link
Contributor Author

mforets commented Feb 24, 2020

Bump?

You could feasibly define width for other equally-sized geometries such as HyperSpheres

fwiw, HyperSpheres have a radius getter function.

Maybe not export it,

I didn't add any export statement in this branch, but width is already exported (https://github.com/JuliaGeometry/GeometryTypes.jl/blob/master/src/GeometryTypes.jl#L186).

@SimonDanisch
Copy link
Member

Oh well^^ Lets merge it then ;)

@SimonDanisch SimonDanisch merged commit 595d187 into JuliaGeometry:master Feb 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants