search

higra / Higra

Hierarchical Graph Analysis
Other
99 stars 19 forks source link

Add the moment of inertia attribute #197

Closed deisemaia closed 4 years ago

codecov[bot] commented 4 years ago

Codecov Report

Merging #197 into master will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #197   +/-   ##
=======================================
  Coverage   98.80%   98.80%           
=======================================
  Files          43       43           
  Lines        4345     4345           
=======================================
  Hits         4293     4293           
  Misses         52       52
PerretB commented 4 years ago

Hi Deise, thank you for the PR. I will look at the code and make a few recommendations if needed.

For the moment can you:

deisemaia commented 4 years ago

Hi Benjamin,

Thank you. Yes, I just updated both files.

On Mon, 11 May 2020 at 17:11, Benjamin Perret notifications@github.com wrote:

Hi Deise, thank you for the PR. I will look at the code and make a few recommendations if needed.

For the moment can you:

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/higra/Higra/pull/197#issuecomment-626765478, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEEEDM2H56UVGC42EV5VGHTRRAITLANCNFSM4M56HTKA .

deisemaia commented 4 years ago

Hi Benjamin,

Concerning the second last remark, which line is to be removed?

On Mon, 11 May 2020 at 22:41, Benjamin Perret notifications@github.com wrote:

@PerretB requested changes on this pull request.

In doc/source/python/tree_attributes.rst https://github.com/higra/Higra/pull/197#discussion_r423285858:

@@ -29,6 +29,7 @@ Tree attributes attribute_topological_height attribute_tree_sampling_probability attribute_volume

  • attribute_moment_of_inertia

incorrect spacing + move the function name in the list to respect the alphabetical sort

In doc/source/python/tree_attributes.rst https://github.com/higra/Higra/pull/197#discussion_r423286046:

\ No newline at end of file +.. autofunction:: higra.attribute_volume + +.. autofunction:: higra.attribute_moment_of_inertia

move the function name in the list to respect the alphabetical sort

In higra/attribute/tree_attributes.py https://github.com/higra/Higra/pull/197#discussion_r423299274:

@@ -675,3 +675,43 @@ def attribute_topological_height(tree): hg.Accumulators.max)

 return res

+ + + +@hg.argument_helper(hg.CptHierarchy) +@hg.auto_cache +def attribute_moment_of_inertia(tree, altitudes, leaf_graph):

  • """
  • Moment of inertia (first Hu moment) of each node of the given tree.

Please, clearly state that the function only works if leaf_graph is a 2d grid graph. It would also be nice to recall the definition of the 2d moment of inertia used here.

In higra/attribute/tree_attributes.py https://github.com/higra/Higra/pull/197#discussion_r423302340:

+ + + +@hg.argument_helper(hg.CptHierarchy) +@hg.auto_cache +def attribute_moment_of_inertia(tree, altitudes, leaf_graph):

  • """
  • Moment of inertia (first Hu moment) of each node of the given tree.
  • :param tree: input tree (Concept :class:~higra.CptHierarchy)
  • :param altitudes: node altitudes of the input tree
  • :param leaf_graph: graph on the leaves of the input tree (deduced from :class:~higra.CptHierarchy on tree)
  • :return: a 1d array
  • """
  • coordinates = hg.attribute_vertex_coordinates(leaf_graph)

There should be a test to check that leaf_graph is a 2d grid graph and throws a meaningful error if not. Something like:

if not hg.CptGridGraph.validate(leaf_graph) or len(hg.CptGridGraph.get_shape(leaf_graph)) != 2: raise ValueError(...)

In higra/attribute/tree_attributes.py https://github.com/higra/Higra/pull/197#discussion_r423302655:

@@ -675,3 +675,43 @@ def attribute_topological_height(tree): hg.Accumulators.max)

 return res

+ + + +@hg.argument_helper(hg.CptHierarchy) +@hg.auto_cache +def attribute_moment_of_inertia(tree, altitudes, leaf_graph):

  • """
  • Moment of inertia (first Hu moment) of each node of the given tree.
  • :param tree: input tree (Concept :class:~higra.CptHierarchy)
  • :param altitudes: node altitudes of the input tree

To be removed

In higra/attribute/tree_attributes.py https://github.com/higra/Higra/pull/197#discussion_r423302899:

  • M_20_leaves = (x_leaves*2) M_00_leaves
  • M_02_leaves = (y_leaves*2) M_00_leaves
  • M_00 = hg.accumulate_sequential(tree, M_00_leaves, hg.Accumulators.sum)
  • M_01 = hg.accumulate_sequential(tree, M_01_leaves, hg.Accumulators.sum)
  • M_10 = hg.accumulate_sequential(tree, M_10_leaves, hg.Accumulators.sum)
  • M_02 = hg.accumulate_sequential(tree, M_02_leaves, hg.Accumulators.sum)
  • M_20 = hg.accumulate_sequential(tree, M_20_leaves, hg.Accumulators.sum)
  • _x = M_10/M_00
  • _y = M_01/M_00
  • miu_20 = M_20 - _x*M_10
  • miu_02 = M_02 - _y*M_01
  • I_1 = (miu_20 + miu_02)/(M_00**2)
  • return I_1

missing newline at the end of file

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/higra/Higra/pull/197#pullrequestreview-409488126, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEEEDMYI6LOCBLPPKPTQOG3RRBPJPANCNFSM4M56HTKA .

PerretB commented 4 years ago 
:param altitudes: node altitudes of the input tree

as the parameter altitudes is not used (and thus should be removed)

PerretB commented 4 years ago

Thank you!