Datseris commented 2 years ago

This PR is a complete overhaul of the API used to declare how recurrence matrices identify recurrences. It now uses a method dispatch on an argument that subtypes AbstractRecurrenceType.

The design based on the discussion surrounding: https://github.com/JuliaDynamics/RecurrenceAnalysis.jl/pull/134#issuecomment-1132236014

What is done:

Overhaul of recurrence specification type
Deprecation of specifying metrics by string
Deprecations of all previous versions

What remains to be done:

[x] Identify why GlobalRecurrenceRate tests fail (completely, as if the method doesn't work)
[x] Fixed skeletonized tests
[x] Re-write tests into the new API, and move old API tests into the Deprecated tests
[x] Make sure all remaining tests pass
[ ] ~~Add update message to package a la DynamicalSystems.jl~~

@heliosdrm or @hkraemer I would appreciate if you gave a formal full review to this PR. The best way to review this PR is to first read the NEWS.md, and then the matrices.jl file. It is not useful to see the GitHub diff, because it is massive.

@heliosdrm The @windowed macro is something that also doesn't sit very well with me. Its source code is just incredibly complicated. Why do we even need a macro for this, when it seems like a function could do the job just as well. Furthermore, why is the source code so complicated and why does it repeat so many things that are existing elsewhere in the library? It even repeats the definition of the RQA dictionary. It would be a very good idea to overhaul the windowed behavior as well, and make it a simpler function with shorter and clearer source code. But this will be done in a different PR of course. For now, I need your help fixing the windowed-related errors because I frankly it would take me too long to understand the source code.

@pucicu how do you calculate the global recurrence rate in your software? I saw that in our source we actually use the "Scaled" functionality to find the recurrence rate via the quantiles of the distance matrix. Maybe you do it more efficiently?

Heads up: this PR was really a lot of effort. It is very likely that I've missed things even though I've tried to make it 100% non-breaking.

Datseris commented 2 years ago

cc @heliosdrm @hkraemer @pucicu

heliosdrm commented 2 years ago

Thanks @Datseris . I'll need some time, after passing some deadlines, before revising this. But I take the assignment, so feel free to bump if you see it's taking too long!

hkraemer commented 2 years ago

Thanks @Datseris , I'll check the tests and take a look why it is not working, but not before next week. Maybe even in the second week of June unfortunately :/

Datseris commented 2 years ago

@heliosdrm and @hkraemer , thank you! But please do not forget that the most important thing is to review the design and documentation first, and then we can move to fixing the tests!

Datseris commented 2 years ago

bump @heliosdrm @hkraemer

Datseris commented 2 years ago

Okay, I'll assume everyone is happy with the proposed API, and I will work to fix broken tests, then merge, and then tag a new release.

Datseris commented 1 year ago

I also did a COMPLETE REWORK OF THE TEST SUITE TO BE "GOOD SCIENTIFIC CODE TESTS". See this issue: https://github.com/JuliaDynamics/DelayEmbeddings.jl/issues/109 for a description of what good tests are. Now, all tests are analytic.

Datseris commented 1 year ago

@heliosdrm I am removing the windowed macro. Unless I am mistaken, one can replace

  rmat = RecurrenceMatrix(...)
  @windowed determinism(rmat, theiler=2, lmin=3) width=1000 step=100

with

  width = 1000
  step = 100
  windows = 1:step:(size(rmat, 1)-width)
  map(1:length(windows)) do i
      rmat_view = view(
        rmat,
        windows[i]:(windows[i]+width),
        windows[i]:(windows[i]+width)
      )
      determinism(rmat_view; theiler=2, lmin=3)
  end

this saves us having to maintain this incredibly convoluted codebase for the macro, which seems to be doing something rather trivial any user can do on their own. I guess you can do a simple Pull Request that offers a windowed function, that does the above loop.

Datseris commented 1 year ago

actually I just wrote and added the windowed function.

codecov-commenter commented 1 year ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (main@52429f3). Click here to learn what that means. The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #135   +/-   ##
=======================================
  Coverage        ?   54.42%           
=======================================
  Files           ?       14           
  Lines           ?      893           
  Branches        ?        0           
=======================================
  Hits            ?      486           
  Misses          ?      407           
  Partials        ?        0

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

JuliaDynamics / RecurrenceAnalysis.jl

Next major release: Rework specification of recurrences + update to DynamicalSystems.jl v3 #135

Codecov Report