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

Rename get_id #9

Open
cortner opened this issue May 25, 2024 · 5 comments
Open

Rename get_id #9

cortner opened this issue May 25, 2024 · 5 comments

Comments

@cortner
Copy link
Member

cortner commented May 25, 2024

this is too generic a name and should be replaced, e.g. with get_particle_id or similar.

@cortner
Copy link
Member Author

cortner commented Aug 10, 2024

it seems reviewing this terminology will be part of AB0.4 so shouuld wait for this.

@cortner
Copy link
Member Author

cortner commented Aug 28, 2024

Now that AB0.4 is ready, I suggest that we replace get_id with species, or at a minimum to use species as a fall-back for get_id.

I am not a big fan of using integers as categorical variables since it is then very easy to get mixed up with indices or even do arithmetic on them when one really shouldn't. Big course of bugs ...

@tjjarvinen
Copy link
Collaborator

In general allowing user to define what to use would be good. But for a default species is better than atomic_number (current default). We can discuss tomorrow how to do this.

@cortner
Copy link
Member Author

cortner commented Aug 28, 2024

The way I see it, if I want to use an integer in my package, or a different "particle id", I can always internally do a conversion e.g. atomic_number(species).

My concern is that get_id seems fundamentally the same function as species. In other words, having both looks like increased flexibility but it really is just defining the same function twice with different names.

Maybe think about a concrete use-case where my perspective doesn't apply and explain it later in our meeting.

@cortner
Copy link
Member Author

cortner commented Aug 28, 2024

AS DISCUSSED: remove get_id and replace with species

@cortner cortner mentioned this issue Sep 3, 2024
2 tasks
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

2 participants