MIPLabCH / nigsp

NiGSP: a module for graph signal processing on neuroimaging data
https://nigsp.readthedocs.io/en/latest/
Apache License 2.0
18 stars 12 forks source link

Add smoothness metric #72

Open hugofluhr opened 1 year ago

hugofluhr commented 1 year ago

Closes #71

PR that add smoothness computation for signals that are defined on nodes of the graph. It's my first contribution to such a project so I'm happy to get feedback :)

Proposed Changes

Change Type

Checklist before review

codecov[bot] commented 1 year ago

Codecov Report

Merging #72 (854c047) into master (4ffa7cb) will increase coverage by 0.13%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #72      +/-   ##
==========================================
+ Coverage   90.93%   91.06%   +0.13%     
==========================================
  Files          14       14              
  Lines        1235     1254      +19     
==========================================
+ Hits         1123     1142      +19     
  Misses        112      112              
Files Changed Coverage Δ
nigsp/cli/run.py 97.14% <100.00%> (+0.08%) :arrow_up:
nigsp/objects.py 100.00% <100.00%> (ø)
nigsp/operations/metrics.py 100.00% <100.00%> (ø)
nigsp/references.py 100.00% <100.00%> (ø)
venkateshness commented 1 year ago

Looks good to me !

Some nice-to-have things: a) Worth embedding a ref paper e.g: https://www.nature.com/articles/srep42013.pdf in the docstring b) Compare the size of nodes in laplacian and signal and raise an error message if there's a mismatch; before the np.dot()

hugofluhr commented 1 year ago

Thanks for the inputs!

Progress tracker :

hugofluhr commented 1 year ago

@smoia the coverage is decreasing partly due to LGR.warning not being covered in tests, any guidance on how to cover those?

venkateshness commented 1 year ago

Unless I am missing (had a quick look on my mobile), LGF.warning is triggered by if statements in 2 cases. You can push to kick these if statements in the break tests ?

smoia commented 1 year ago

If the coverage decreases a little, that's ok. If you do want to test particular cases, I would add a couple of asserts in the main test of the function that do specific checks on those cases!

Break tests are only for raising errors!

smoia commented 1 year ago

@hugofluhr can we merge this in?