Skip to content

Changes to the sensors.py file in the LightDependentGECI class returned the GECI class to the previous version found on the main github as well.#67

Open
spoma3 wants to merge 9 commits intosiplab-gt:masterfrom
spoma3:master

Conversation

@spoma3
Copy link

@spoma3 spoma3 commented Oct 30, 2025

Made changes to the code to get the parameter values from the Dictionaries dictionaries instead of from having to read json files each time.
made changes to the dictionary
Made changes to the LightDependentGECI code to make sure that the LightDependentGECI objects can be created and used
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests should go in tests/imaging/test_sensors.py

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, this is more of an interactive script for testing parameters...this would go better in an if __name__ == 'main': block at the bottom. Or maybe in notebooks?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should go in docs/tutorials

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If these all have the same variables, this would be way more readable as a CSV

"""Defines ``exc_factor``"""

model: str
model: str = field(default="exc_factor = 1 : 1", init=False)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to put this here, since this is defined in NullExcitationModel

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and LightExcitation is where you'd put your Hill function. Looks like you had it then removed it again?


pass

"""Uses a Hill equation to convert from Ca2+ to ΔF/F, as in Song et al., 2021"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of this stuff should be defined on LightExcitation, not on LightDependentGECI. This keeps the parameter sets for each part of the model nice and separate. Look at DynamicCalcium or DoubExpCalBindingActivation for examples

if callable(super_post_init):
super_post_init()

def fluorescence(self, GECI_str, brightness, wavelength):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks like a testing sort of function? If the interface is clunky enough we need code like this to test it, we need to revise the interface. But I don't think so...look at tests/test_sensors.py and you'll see it should be as simple as instantiating it (e.g., geci = gcamp7f(), then injecting it.

print(f"ng.dFF is: {ng.dFF}")
return ng.dFF

def LightDependentParams(GECI_str) -> dict:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we definitely don't want this function: the structure for what params a LightDependentGECI needs is already captured by the class and the model attributes. Look at geci() and you'll see the machinery is already in place to simply load from a bunch of keyword parameters (including the LightExcitation model), which you could load from a CSV using pandas

k = params['k']
return {'A' : A, 'baseline' : baseline, 'k' : k, 'n' : n, 'ec50' : ec50}, params_w

def light_dependent_geci(name: str, doub_exp_conv: bool, pre_existing_cal: bool, bind_act_model: bool, x, w, Ca_rest=50 * nmolar,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, again, just use the geci() function I already made

@kjohnsen
Copy link
Member

In general, it seems like you're trying to reinvent the wheel. I already had the structure in place to make this easy.

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

Successfully merging this pull request may close these issues.

2 participants