-
Notifications
You must be signed in to change notification settings - Fork 46
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
Vertex name changes #346
Vertex name changes #346
Conversation
vertices._LinearCageVertex.init_at_center, | ||
vertices._NonLinearCageVertex.init_at_center, | ||
vertices._UnaligningVertex.init_at_center, | ||
vertices.LinearVertex.init_at_center, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now that the vertices are public, I think they should be accessed in tests by the public API, not by this "unofficial" API. so stk.cage.LinearVertex
not vertices.LinearVertex
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree and have replaced all vertices = stk.cage.vertices
(for example) with direct calls to stk.cage.VERTEXNAME
.
I also have some concerns about exposing all of the vertex classes. I'm not against it, but I guess I would like to know what the arguments for and against this are? For example, what is the cost / benefit of exposing |
I see your concern and my main argument is how do you decide which to make public and which not to? There is no technical reason I see for that other than the desire to minimise access until needed. For example, why make the cage vertex accessible, but not the host vertex? A user may be just as inclined to use that in their own code as an |
I guess with cages the extensibility argument is a bit easier. The amount of cage topologies we have already, means that we have a pretty good understanding of how this vertex can be used to easily build new topology graphs. With something like host guest, we have experience in using it in 1 topology graph. so the reusability of it a lot less well understood. I guess I'm not convinced that just because a user wants to use that vertex, that I want to allow them to, because doing so would remove my flexibility to modify it in the future. Having said that, I think we can expose them because removing them can be easy for the users to fix (just copy paste the code into their own scripts). So I think it is a worthwhile experiment. |
docs/source/basic_examples.rst
Outdated
class NewCageTopology(stk.cage.Cage): | ||
|
||
_vertex_prototypes = ( | ||
stk.cage.LinearVertex(0, [0, 0, 0]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I think there is a type error here. the position should be a tuple
of float
so (0., 0., 0.)
I realize that this type error exists in the actual code, but I don't want you to change that. This PR just needs to make sure that users get a fully correct example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Related Issues: #344
Requested Reviewers: @lukasturcani
This PR makes all vertex classes (except for Cage and Cof base classes) publicly accessible by simply changing class names. Also removes designation of Cage and Cof types within name, as that is designated by the access point. (sorry if my terminology here is wrong). An example of accessing the vertices was added.
The following example shows that this works.