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

Avoid use of getters with symbols #102

Closed
Tracked by #101
cortner opened this issue May 19, 2024 · 3 comments
Closed
Tracked by #101

Avoid use of getters with symbols #102

cortner opened this issue May 19, 2024 · 3 comments

Comments

@cortner
Copy link
Member

cortner commented May 19, 2024

I'm nervous about the use of getters using the symbols describing the fieldnames. To my mind this defeats the entire point of an abstract interface. I appreciate the elegance those with which one can access properties of a system. I therefore want to propose to replace calls such as

sys[inds, :position] 

with

sys[inds, position] 

This can be formally equivalent to position(sys[inds]) but internally something more efficient can be done. Because position has its own type this means it can be done in a type-stable way, where as using a Symbol one relies on constant-propagation. (I don't know how reliable this is?)

But the biggest advantage for me is that this way one can change the internals without changing the way we access.

A prototype of this is already in #101

@cortner cortner mentioned this issue May 19, 2024
13 tasks
@rkurchin
Copy link
Collaborator

I'm pretty sure I agree with you. Just to make sure I understand what you're getting at here, in your second proposed option, position is now simply the function position rather than the name of a field in the object? Definitely agree that we should avoid explicitly referencing those, for precisely the reason you point out.

@mfherbst
Copy link
Member

Ok for me.

@cortner
Copy link
Member Author

cortner commented Jul 1, 2024

Interestingly, the discussion on the MD document went against what we agreed in this issue. Do you want to close it?

@mfherbst mfherbst closed this as not planned Won't fix, can't repro, duplicate, stale Jul 2, 2024
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

No branches or pull requests

3 participants