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.

Sign up for GitHub

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

Jump to bottom

[MRG] ENH: add average_dipoles function to Dipole #156

Merged
merged 4 commits into from Aug 28, 2020
Merged

[MRG] ENH: add average_dipoles function to Dipole #156

merged 4 commits into from Aug 28, 2020

Conversation

blakecaldwell
Copy link
Member

Split from #83 for a fast merge

@jasmainak
Copy link
Collaborator

jasmainak commented Aug 27, 2020 •
edited

what do you think of adding an attribute nave that is 1 by default. If it's not equal to 1, then we raise an error in average_dipole and a warning in write.

@codecov-commenter
Copy link

Codecov Report

Merging #156 into master will decrease coverage by 0.26%.
The diff coverage is 20.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #156      +/-   ##
==========================================
- Coverage   70.31%   70.04%   -0.27%     
==========================================
  Files          19       19              
  Lines        1984     1993       +9     
==========================================
+ Hits         1395     1396       +1     
- Misses        589      597       +8
Impacted Files Coverage Δ
hnn_core/dipole.py 63.01% <11.11%> (-7.30%) ⬇️
hnn_core/__init__.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f14251...68343f4. Read the comment docs.

jasmainak
jasmainak reviewed Aug 27, 2020
View reviewed changes
hnn_core/dipole.py Outdated
@@ -69,6 +69,40 @@ def read_dipole(fname, units='nAm'):
return dpl


def average_dipoles(dpls):
"""Compute dipole averages over a list of Dipole objects. 'L2', 'L5' and 'agg'
components are averaged separately.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick, should be single line

jasmainak
jasmainak reviewed Aug 27, 2020
View reviewed changes
hnn_core/dipole.py Outdated
average over the same components in the input list
"""
# need at least one Dipole to get times
if (len(dpls) < 2):
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick, paranthesis not needed.

@jasmainak
Copy link
Collaborator

Otherwise looks good. Let me know what you think of the nave or n_averages attribute

@blakecaldwell
Copy link
Member Author

Otherwise looks good. Let me know what you think of the nave or n_averages attribute

Does it get incremented by average_dipole? In #83 you said "update", but I can only see utility if it is incremented.

@jasmainak
Copy link
Collaborator

If you averaged 5 dipoles, I would set nave to 5 in the returned Dipole object. So yeah it's incremented, don't see a situation where it would decrement.

Blake Caldwell added 2 commits August 27, 2020 19:34
MAINT: add nave attribute to dipole
1134cdb 
Prevents accidentally reaveraging trials with an inconsistent basis
TST: also check that nave > 1 raises exception
27f2b3e
@blakecaldwell blakecaldwell changed the title ENH: add average_dipoles function to Dipole [MRG] ENH: add average_dipoles function to Dipole Aug 27, 2020
@blakecaldwell
Copy link
Member Author

blakecaldwell commented Aug 27, 2020 •
edited

If you averaged 5 dipoles, I would set nave to 5 in the returned Dipole object. So yeah it's incremented, don't see a situation where it would decrement.

Otherwise looks good. Let me know what you think of the nave or n_averages attribute

I think it's a good addition. Although I didn't add it to write(). Creating an issue for later...

jasmainak
jasmainak approved these changes Aug 27, 2020
View reviewed changes
Copy link
Collaborator

@jasmainak jasmainak left a comment

Choose a reason for hiding this comment

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

+1 for merge from my end

don't know if @rythorpe wants to look, otherwise I can merge once CIs come green

jasmainak
jasmainak reviewed Aug 27, 2020
View reviewed changes
doc/whats_new.rst
@@ -47,6 +47,8 @@ API

- Make a context manager for parallel backends (JoblibBackend, MPIBackend), by `Blake Caldwell`_ in `#79 <https://github.com/jonescompneurolab/hnn-core/pull/79>`_

- Add average_dipoles function to `hnn_core.dipole`, by `Blake Caldwell`_ in `#156 <https://github.com/jonescompneurolab/hnn-core/pull/156>`_
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can make the function clickable here by doing :func:hnn_core.average_dipoles.

@blakecaldwell blakecaldwell mentioned this pull request Aug 27, 2020
Closed
2 tasks
@rythorpe
Copy link
Contributor

Looks good to me. Thanks @blakecaldwell!

@jasmainak jasmainak merged commit 762cec5 into jonescompneurolab:master Aug 28, 2020
@jasmainak
Copy link
Collaborator

I'm going to merge this. The whats_new issue can be fixed in another PR, not urgent. Thanks @blakecaldwell !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasmainak jasmainak jasmainak approved these changes

@rythorpe rythorpe rythorpe approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@blakecaldwell @jasmainak @codecov-commenter @rythorpe