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

New interface for channels with cell.add_global_param() #474

Open
michaeldeistler opened this issue Oct 28, 2024 · 13 comments
Open

New interface for channels with cell.add_global_param() #474

michaeldeistler opened this issue Oct 28, 2024 · 13 comments
Assignees

Comments

@michaeldeistler
Copy link
Contributor

michaeldeistler commented Oct 28, 2024

cell.add_global_param("e_k", -80.0)
cell.add_global_state("e_ca", -80.0)

then e_k can be accessed via params["e_k"].

@jnsbck
Copy link
Contributor

jnsbck commented Oct 29, 2024

I dont quite understand this. could you elaborate? :)

@michaeldeistler
Copy link
Contributor Author

Ah, yes, I created this issue in a rush during a zoom meeting and forgot to add more details.

We got feedback that it is unintuitive that things like reversal potentials or temperature are channel_params. Instead, they should simply be a property of the membrane. I am not sure yet how to best implement this, but the above was my initial idea.

@michaeldeistler
Copy link
Contributor Author

michaeldeistler commented Dec 12, 2024

We (@kyralianaka @huangziwei @jnsbck) decided on the following:

The add_membrane_param() method will replace global channel params.

This implies the following:

  1. All parameters and states within a channel are strictly local
  2. We can get rid of f"{prefix}_gNa" and just call it gNa.
  3. We have a .prefix for every channel which will be used to generate the name of the parameter in self.nodes.
  4. Any parameter that is global across channels must be added via add_membrane_param(). We will not provide default values for any global membrane params or states.
  5. To distinguish between params and states, we will have add_membrane_param() and add_membrane_state().

Additional improvements to Channel:

  • rename channel_params to params
  • ensure that dt and delta_t are consistent and the ordering of input args is consistent. We suggest: v, states, params, dt.
  • rename _name to name

@michaeldeistler michaeldeistler changed the title cell.add_membrane_property() New interface for channels with cell.add_membrane_param() Dec 12, 2024
@michaeldeistler
Copy link
Contributor Author

@r-makarov see above---we are sketching out a new structure for channels. If you have any feedback, that would be great!

@michaeldeistler michaeldeistler changed the title New interface for channels with cell.add_membrane_param() New interface for channels with cell.add_global_param() Dec 12, 2024
@jnsbck jnsbck self-assigned this Dec 12, 2024
@jnsbck
Copy link
Contributor

jnsbck commented Dec 13, 2024

While we're at it, we should rename all delta_t to dt for consistency's sake

@michaeldeistler
Copy link
Contributor Author

michaeldeistler commented Dec 13, 2024

or the other way around :) Linters complain about dt, but not about delta_t.

(but I think I also tend towards dt.)

@r-makarov
Copy link

@michaeldeistler Sounds great! I'm in favor of all the proposed changes.

  1. I would suggest avoiding duplication in names like Na_gNa in general, including in the parameter names within self.nodes. I would use gbar in this case, so it would become Na_gbar.
  2. Isn't this the same as the channel's .name? Maybe it could be used directly? For consistency with NEURON notation (e.g. for some automated model construction), I would consider adding the name as a suffix rather than a prefix.
  3. To ensure consistency between models, I suggest providing recommended parameter names, even though you won't set default values for these parameters.

@jnsbck
Copy link
Contributor

jnsbck commented Dec 13, 2024

These suggestions are great!
2. I agree that the channels currently in jaxley should do this!
3. Not necessarily. The purpose of the name attr. is mainly for the user to be able to change how the channel shows up in nodes, i.e.

slow_K = K().change_name("slow_K") 
cell.insert(slow_K)
  1. I.e. write a small section on the website about naming conventions or if you meant something else could you give an example?

Another thing I just noticed: The methods in channel / synapse are called update_states and init_state. I think we should settle for either singular or plural.

@r-makarov
Copy link

Thanks!

  1. Based on Michael's comment above, I thought that the prefix generates the parameter name in self.nodes. When renaming a channel, would users need to modify both the name and prefix attributes?
  2. Yes, that would be helpful to document on the website. In addition, probably these parameters can be added to the params dictionary with their values set to None.

Have you made any decision regarding the temperature adjustment factor tadj? Should it be computed in the init_params method? Is it going to be accessed as a parameter or state or just a channel’s attribute directly? Also, when users change global parameters like temperature, would they need to re-run init_params for all channels e.g. to calculate new tadj?

@jnsbck
Copy link
Contributor

jnsbck commented Feb 24, 2025

More thoughts I had thinking about the new channel API:

  • Make Synapses, Channelss and Pumpss inherit from Mechanism.
  • I have somewhat changed my mind about all the prefixing in the mechanisms. While being a bit messy it does a good job of making explicit what is happening (we could still add a few utils that makes unpacking params a bit nicer). Also it's very clear when parameters show up under some name in nodes that it was user intended, which might be confusing if internally params were to be prefixed w.o. users knowledge.

Incl. all the feedback above, here is the following proposal.

class Mechanism:
    name = None
    params: dict = None
    states: dict = None
    current_subscript: str = None
    META: dict = None

    def __init__(self, name = None):
        self.name = self.__class__.__name__.lower() # <- replace property by attr, force lowercase
        self.params = {} # <- all mechs have params and states
        self.states = {} # will make code much nicer, since mech.params works for all mechs
        self.current_subscript = self.name # <- rename subscript to make clear that current will be named "i_name"
        self.META = {} # <- I think we could make this a default attr for all mechs

        if name is not None:
            self.change_name(name)

    @property
    def current_name(self): # retain the old current name, ensures all mech currents are of form "i_subscript"
        return f"i_{self.current_subscript}"
    
    def change_name(self, new_name):
        for params_or_states in [self.params, self.states]:
            for key in params_or_states:
                if key.startswith(self.name):
                    new_key = key.replace(self.name, new_name)
                    params_or_states[new_key] = params_or_states.pop(key)

        if self.current_subscript.startswith(self.name):
            self.current_subscript = self.current_subscript.replace(self.name, new_name)

        self.name = new_name
        return self

    @abstractmethod
    def update_states(self, t, states, params, v, dt): # <- standardize order of args; add t for consistency
        # convention usually is: (t, states, args) / (t, states, (params, v, dt))
        return {}

    @abstractmethod
    def compute_current(self, t, states, params, v):
        raise NotImplementedError

    @abstractmethod
    def init_states(self, t, states, params, v, dt): # <- rename to init_state**s**
        return {}
    
    @abstractmethod
    def init_params(self, t, states, params, v, dt): # <- add init_params (see #507)
        return {}
    
    def _vectorfield(self, t, states, params, v): # <- make vectorield optionally accessible
        return {}
    

class Channel(Mechanism):
    def __init__(self, name = None):
        super().__init__(name)


class Leak(Channel):
    def __init__(self, name = None):
        super().__init__(name)
        self.params = {f"g_{self.name}": 0.01, f"E_{self.name}": -70.0}
        
    def compute_current(self, t, states, params, v):
        g, E = params[f"g_{self.name}"], params[f"E_{self.name}"]
        return g * (v - E)
        
    def update_states(self, t, states, params, v, dt):
        return {}
    
    def init_states(self, t, states, params, v, dt):
        return {}

Lemme know your thoughts!

@michaeldeistler
Copy link
Contributor Author

nice, I like it!

Two things:

  1. I am not sure about the way of defining current_name. To me, this is a bit convoluted and requires somewhat advanced knowledge of Python to understand. We will want also people who do not know what a property in Python is to be able to understand Jaxley. As such, I would just define self.current_name = .... To me that's simpler and equally flexible.
  2. Before starting to implement it, we should also sketch out the Pump (with an example) and settle on how to deal with ion_name.

@jnsbck
Copy link
Contributor

jnsbck commented Feb 25, 2025

I disagree with 1. for the following reasons:

  • the user will not need to know about current_name at all
  • current_subscript will ensure standardized current_naming, which we can rely on / make use of in the backend. If the user gets to name a current however they want, i.e. "Some_IonName_current" this will make things potentially difficult.

Regarding 2, yes, lets discuss ideas here. Also related to #592.

...to start a discussion.

class Pump(Mechanism):
    def __init__(self, name = None):
        super().__init__(name)
        self.ion = "ion" # <- we could also make this part of mech, since ultimately, Channels etc. also are associated with ions.

Additional thoughst:

  • Should we make lower case a convention for param names?
  • We should write a FAQ about the conventions once finalized and as part of the PR.

@jnsbck
Copy link
Contributor

jnsbck commented Feb 27, 2025

Should we also add a set method to Mechanism?

This would allow:

branch = jx.Branch(ncomps=2)
channel = CustomChannel()

# insert channel into first comp
channel.set("g", 1.0)
branch.comp(0).insert(channel)

# change params and insert into 2nd comp
channel.set("g", 2.0)
branch.comp(1).insert(channel)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants