search

JuliaMolSim / AtomsCalculatorsUtilities.jl

Utilities for implementing AtomsCalculators
MIT License
2 stars 0 forks source link

Rename `get_id` #9

Open cortner opened 3 months ago

cortner commented 3 months ago

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

cortner commented 2 weeks ago

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

cortner commented 2 days ago

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 commented 2 days ago

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 commented 1 day ago

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 commented 1 day ago

AS DISCUSSED: remove get_id and replace with species