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

.comp and .loc addresses #285 #288

Closed
wants to merge 1 commit into from
Closed

.comp and .loc addresses #285 #288

wants to merge 1 commit into from

Conversation

lappalainenj
Copy link
Contributor

This introduces comp(index: int) and loc(loc: float). To keep legacy code intact, this will trigger a warning if comp(loc: float) is used but still work. I added a unit test, showing that both methods yield equivalent simulations. This defines _compartment_view, comp, and loc in both Branch and BranchView -- I do not know if having both is desirable?

@lappalainenj lappalainenj changed the title addresses #285 .comp and .loc addresses #285 Mar 14, 2024
Copy link
Contributor

@michaeldeistler michaeldeistler left a comment

Choose a reason for hiding this comment

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

Thanks a lot, I really like how this is done now!

If I understand correctly, currently, the following will not do the same:

nseg = 8

branch1.loc(0.0).set("radius", 1.0)
branch2.comp(0).set("radius", 1.0)

Instead, the following two would do the same thing:

branch1.loc(0.0).set("radius", 1.0)
branch2.comp(nseg-1).set("radius", 1.0)

Your tests work because index_of_loc takes care of this here.

This weird behavior is because of #30, but I think it will be confusing for users. I think that an easy fix should be to compute self.pointer.nseg-index in the adjust_view of CompartmentView and add the nseg-index_of_loc(...) in your tests.

view["global_cell_index"] = view["cell_index"]
return CompartmentView(self, view)
elif key in self.group_nodes:
# if key == "comp":
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for cleaning this up! (please remove the comments)

if index == "all":
pass
else:
# support for legacy code
Copy link
Contributor

Choose a reason for hiding this comment

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

Since it is only 5 people hacking on Jaxley right now I do not think that we have to support legacy code. Please remove this.

"""Return a compartment of the discretized branch.

Args:
index: integer or float between 0 to 1 for
Copy link
Contributor

Choose a reason for hiding this comment

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

Since it is only 5 people hacking on Jaxley right now I do not think that we have to support legacy code. Please remove the legacy option.

@jnsbck
Copy link
Contributor

jnsbck commented Mar 15, 2024

Did not see this PR. Also addressed this in #277, since I needed the functionality to get all my tests to pass. Can revert what I did then.

@lappalainenj I think one only needs to change

if key == "comp":

assert key == "comp"

to key in ["comp", "loc"]

and replace

return CompartmentView(self, view)

by

if key == "loc": return CompartmentView(self, view).loc
else: return CompartmentView(self, view).loc

Also don't forget to change all prev comp to loc incl in tests.

@jnsbck jnsbck mentioned this pull request Mar 15, 2024
@lappalainenj
Copy link
Contributor Author

@jnsbck this is because it also addresses #289. I think it would be cleanest if we first merge your PR #277 and we use this implementation strategy to do #289

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.

3 participants