search

pybamm-team / PyBaMM

Fast and flexible physics-based battery models in Python
https://www.pybamm.org/
BSD 3-Clause "New" or "Revised" License
1.03k stars 518 forks source link

Rename `set_events` #4266

Open MehrdadBabazadeh opened 1 month ago

MehrdadBabazadeh commented 1 month ago

As discussed in the PR #4254, set_event should be renamed because of misleading.

          This adds a side effect to what would be expected to be a simple setter. At minimum it should be changed to `append_events()` or `add_events()`. Was there an actual bug that this was fixing or just not sure if it would cause issues?

_Originally posted by @kratman in https://github.com/pybamm-team/PyBaMM/pull/4254#discussion_r1672332716_

brosaplanella commented 1 month ago

I would also vote for using a dictionary instead of a list, so it is easier to access a specific event in case the user wants to modify in.

kratman commented 1 month ago

I would also vote for using a dictionary instead of a list, so it is easier to access a specific event in case the user wants to modify in.

The one advantage of a list over a dictionary is the ordering in a list. So if we are appending to a list then the events are in some order. We can use an "OrderedDict" object to get both

brosaplanella commented 1 month ago

To be honest, I don't know if the ordering is that important.

valentinsulzer commented 1 month ago

Ordering is not super important, and Python's dict is an OrderedDict since Python 3.6