JuliaDynamics / RecurrenceAnalysis.jl

Recurrence Quantification Analysis in Julia
https://juliadynamics.github.io/RecurrenceAnalysis.jl/stable/
MIT License
45 stars 12 forks source link

Map Inf path lengths to zero #117

Closed gerrygralton closed 3 years ago

gerrygralton commented 3 years ago

Issue opened in #116.

Closes #116

Datseris commented 3 years ago

Thank you :) @gerrygralton Could you please provide a unit test for this scenario? You should put the test in one of the files in the test folder. Reading any file will guide you on how test work.

@heliosdrm or @hkraemer would you kindly review and approve the PR? I do not have the mathematical background :)

heliosdrm commented 3 years ago

Thanks! If you have a specific example of a network with disconected nodes that you want to test, either defined as a SimpleGraph or by its adjacency matrix, you may add it in the last block of test/smallmatrix.jl, where there are other examples that you can follow.

gerrygralton commented 3 years ago

Hi, Thanks for having a look at this! In 127dad8a8de6679d8e09f352e7dd5dab2e1aec15 I've changed the formula slightly to remove the LightGraphs.closeness_centrality dependency because that formulation really doesn't like disconnected graphs. It also has the advantage of being faster. Using the 10x10 example from test/small_matrix.jl:

New:

BenchmarkTools.Trial: 
  memory estimate:  2.67 KiB
  allocs estimate:  5
  --------------
  minimum time:     929.786 ns (0.00% GC)
  median time:      957.500 ns (0.00% GC)
  mean time:        1.104 μs (5.17% GC)
  maximum time:     101.671 μs (98.23% GC)
  --------------
  samples:          10000
  evals/sample:     28

Old:

BenchmarkTools.Trial: 
  memory estimate:  20.08 KiB
  allocs estimate:  165
  --------------
  minimum time:     11.829 μs (0.00% GC)
  median time:      12.584 μs (0.00% GC)
  mean time:        15.399 μs (8.92% GC)
  maximum time:     3.706 ms (98.96% GC)
  --------------
  samples:          10000
  evals/sample:     1

I have also added some tests in 989bd14dd8029873409c986c24f81662a350a91b.

Datseris commented 3 years ago

Thanks, can you please adjust Project.toml version and CHANGELOG.md accordingly?

gerrygralton commented 3 years ago

Thanks, can you please adjust Project.toml version and CHANGELOG.md accordingly?

Uh, there is no CHANGELOG.md...

Datseris commented 3 years ago

Woooooooooooops sorry about that, let's start one then. Just provide the note that changelog starts being kept from current version.

gerrygralton commented 3 years ago

Is NEWS.md the same thing?

Datseris commented 3 years ago

Ah, sorry for the confusion @gerrygralton . Yeap, it is NEWS.md. I got confused, in some other packages it is called CHANGELOG.md...

gerrygralton commented 3 years ago

@Datseris This new version hasn't been tagged because TagBot has turned itself off.

Datseris commented 3 years ago

Oh, okay, do you know how to turn it on again? EDIT: Okay I turned it on again,.