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.

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

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

Merged
merged 47 commits into from
Feb 20, 2023

Conversation

Datseris
Copy link
Member

@Datseris Datseris commented May 21, 2022

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: #134 (comment)

What is done:

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

What remains to be done:

  • Identify why GlobalRecurrenceRate tests fail (completely, as if the method doesn't work)
  • Fixed skeletonized tests
  • Re-write tests into the new API, and move old API tests into the Deprecated tests
  • 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.

There is no reason to test on 4 different datasets, because we do not actually use the different structures of the datasets in the tests.
@Datseris
Copy link
Member Author

cc @heliosdrm @hkraemer @pucicu

@heliosdrm
Copy link
Collaborator

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
Copy link
Contributor

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
Copy link
Member Author

@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!

@hkraemer hkraemer self-requested a review June 27, 2022 14:08
@Datseris
Copy link
Member Author

bump @heliosdrm @hkraemer

@Datseris
Copy link
Member Author

Datseris commented Sep 9, 2022

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 Datseris changed the title Rework specification of recurrences Next major release: Rework specification of recurrences + update to DynamicalSystems.jl v3 Feb 17, 2023
@Datseris
Copy link
Member Author

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

@Datseris
Copy link
Member Author

Datseris commented Feb 20, 2023

@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
Copy link
Member Author

actually I just wrote and added the windowed function.

@codecov-commenter
Copy link

Codecov Report

❗ 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           

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

@Datseris Datseris merged commit d3a21ee into main Feb 20, 2023
@Datseris Datseris deleted the rework_recurrence_type branch February 20, 2023 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants