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

Implement LFPy backend #385

Merged
merged 51 commits into from
Dec 8, 2022
Merged

Implement LFPy backend #385

merged 51 commits into from
Dec 8, 2022

Conversation

alejoe91
Copy link
Contributor

@alejoe91 alejoe91 commented Mar 1, 2022

LFPy-related changes

  • Added LFPy as a backend for simulation
  • Added extracellular efeatures class extraFELFeature that supports computing features from extracellular "template" (avg action potential). Features include:
    • "peak_to_valley": duration between negative and positive peak
    • "halfwidth": full width alf maximum of negative peak
    • "peak_trough_ratio": ratio between peak and trough
    • "repolarization_slope": slope from the first peak to baseline
    • "recovery_slope": slop from second peak to baseline
    • "neg_peak_relative": relative amplitude of negative peaks with respect to max channel
    • "pos_peak_relative": relative amplitude of positive peaks with respect to max channel
    • "neg_peak_diff": relative timing of negative peaks with respect to max channel
    • "pos_peak_diff": relative timing of positive peaks with respect to max channel
    • "neg_image": normalized potential at time of negative peak
    • "pos_image": normalized potential at time of positive peak

Other changes

  • Add param_dependancies in parameters
  • Add **morhp_modifiers_kwargs to Morphology
  • Implement NrnSegmentSectionDistanceScaler (base class also for NrnSegmentSomaDistanceScaler)

Copy link
Contributor

@DrTaDa DrTaDa left a comment

Choose a reason for hiding this comment

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

The setup.py needs to be updated to include LFPy.
Also we will need tests to maintain the test coverage above 90%.

@alejoe91
Copy link
Contributor Author

The setup.py needs to be updated to include LFPy. Also we will need tests to maintain the test coverage above 90%.

It was not added on purpose to make the LFPy dependency weak. All LFPy imports are local so one can still run normal BPO without having LFPy installed

@DrTaDa
Copy link
Contributor

DrTaDa commented Jul 14, 2022

The setup.py needs to be updated to include LFPy. Also we will need tests to maintain the test coverage above 90%.

It was not added on purpose to make the LFPy dependency weak. All LFPy imports are local so one can still run normal BPO without having LFPy installed

LFP needs to be in an extra require then:

extras_require={
        "LFPy": ["LFPy"]
}

@alejoe91
Copy link
Contributor Author

yeah that could work!

tox.ini Outdated Show resolved Hide resolved
bluepyopt/parameters.py Outdated Show resolved Hide resolved
@wvangeit
Copy link
Contributor

So it seems LFPy requires mpi4py? Installation seems to fail during the tests. Can we disable mpi4py in LFPy, since I assume we don't need it?

@alejoe91
Copy link
Contributor Author

So it seems LFPy requires mpi4py? Installation seems to fail during the tests. Can we disable mpi4py in LFPy, since I assume we don't need it?

Unfortunately it's a requirement: https://github.com/LFPy/LFPy/blob/master/requirements.txt

@wvangeit
Copy link
Contributor

Could we ask to get it removed from the hard requirements (or to make an extra that doesn't require it?). I'm a bit reluctant to make mpi4py a requirement for the bpopt tests if it is not required.

@alejoe91
Copy link
Contributor Author

Could we ask to get it removed from the hard requirements (or to make an extra that doesn't require it?). I'm a bit reluctant to make mpi4py a requirement for the bpopt tests if it is not required.

LFPy/LFPy#429

@DrTaDa
Copy link
Contributor

DrTaDa commented Jul 25, 2022

@alejoe91 If you agree, I will rollback the changes to parameters,py and parameterscalers.py.
We were using them for the Hallermann model but it is not use anymore.
It will also require us to do some changes on the multimodal fitting repo such as removing https://github.com/alejoe91/multimodalfitting/blob/cb49b63d0e76433448b9e4ca6ca825cf74598ffa/multimodalfitting/models/model.py#L305

@DrTaDa
Copy link
Contributor

DrTaDa commented Jul 25, 2022

@alejoe91 thinking about it more, it might be useful for someone else some day. I am not sure what should be done.

"""Instantiate"""

for stimulus in self.stimuli:
stimulus.instantiate(sim=sim, icell=icell)
if isinstance(stimulus, LFPStimulus):
stimulus.instantiate(lfpy_cell=cell_model.lfpy_cell)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be sim, lfpy_cell

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AurelienJaquier can you take care of it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AurelienJaquier can you take care of it?

actually I'm on it ;)

alejoe91 and others added 7 commits July 29, 2022 16:57
  *  turn print statements in eFELFeature into logger.info
  *  change docstring description of exp_mean and exp_std in extraFELFeature
  *  remove MEA dependency in setup
  *  use custom lfpy from fork in setup
  *  use lfpy from extras in tox
  *  add test function for masked_cosine_distance
@espenhgn
Copy link

Hi @alejoe91, @wvangeit; as mpi4py appeared to be a problem, I've made a PR to LFPy (LFPy/LFPy#433) that removes this package dependency altogether (by using similar MPI communication provided via neuron.h.ParallelContext()). Seems to work in my own testing but it should be tested also with this PR before merging.

@wvangeit
Copy link
Contributor

There are just some formatting issues at the moment.

@AurelienJaquier
Copy link
Collaborator

I think the formatting issues have been solved in the master branch of BlueBrain/BluePyOpt. So a merge branch 'master' into 'lfpy' should solve them.

@codecov-commenter
Copy link

codecov-commenter commented Dec 6, 2022

Codecov Report

Merging #385 (1e56736) into master (519b2fc) will decrease coverage by 0.69%.
The diff coverage is 86.18%.

@@            Coverage Diff             @@
##           master     #385      +/-   ##
==========================================
- Coverage   89.53%   88.84%   -0.70%     
==========================================
  Files          34       35       +1     
  Lines        2361     2977     +616     
==========================================
+ Hits         2114     2645     +531     
- Misses        247      332      +85     
Impacted Files Coverage Δ
bluepyopt/ephys/morphologies.py 96.51% <75.00%> (+0.12%) ⬆️
bluepyopt/ephys/stimuli.py 95.23% <76.66%> (-4.77%) ⬇️
bluepyopt/ephys/models.py 86.36% <78.81%> (-4.74%) ⬇️
bluepyopt/ephys/simulators.py 86.95% <80.64%> (-2.33%) ⬇️
bluepyopt/ephys/efeatures.py 85.45% <83.06%> (-4.09%) ⬇️
bluepyopt/ephys/parameters.py 88.46% <83.33%> (-1.34%) ⬇️
bluepyopt/ephys/responses.py 86.66% <87.50%> (+0.30%) ⬆️
bluepyopt/ephys/parameterscalers.py 91.13% <88.57%> (-0.39%) ⬇️
bluepyopt/ephys/protocols.py 83.44% <93.33%> (+0.46%) ⬆️
bluepyopt/ephys/recordings.py 97.22% <94.44%> (-2.78%) ⬇️
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@wvangeit
Copy link
Contributor

wvangeit commented Dec 6, 2022

Should we merge this, @alejoe91 @DrTaDa @AurelienJaquier ?

@DrTaDa
Copy link
Contributor

DrTaDa commented Dec 6, 2022

Yes !

@wvangeit wvangeit merged commit 3440402 into BlueBrain:master Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants