Skip to content

Conversation

@heliosdrm
Copy link
Contributor

Following the recommendation in: https://discourse.julialang.org/t/ann-announcing-slidingdfts-jl/123793

This is an adaptation of https://github.com/heliosdrm/SlidingDFTs.jl for FourierTools. Some points to note:

  • This PR actually contains two things: an interface to use and develop SDFTs, and the actual implementation of SDFT methods. Accordingly, two files are included in src: sdft_interface.jl (with a module SlidingDFTs), and sdft_implementations.jl. The documentation page deals with both of them in different sections.
  • Only the most basic method is implemented for the time being. At least the rSDFT will follow if this is approved.

@heliosdrm
Copy link
Contributor Author

There may be details of this implementation that require an explanation. Please ask as needed.

To start with, one might ask about the design of SlidingDFTs.dataoffsets. It is meant to return the offsets that can be used in the calls to SlidingDFTs.previousdata for a given SDFT method, but in reality the only that matters is whether it returns nothing or anything else. So why?

The answer is that the whole design of the interface to create SDFT methods was devised before implementing it, and at that time I planned two different kinds of SDFT implementations:

  1. One suitable for input data that are stateful iterators (the one that is currently coded). The states of SDFT iterators store an array with all previous values of the input data that may be used in future iterations: for the i-th iteration of the SDFT, this is the input data samples from its i-th position to its i+n-th position, where n = SlidingDFTs.windowlength(method).

  2. Another that would store less data, but would not be suitable for stateful iterators. Instead of the full fragment of the input between i and i+n, it might store a smaller set of data based on the output of SlidingDFTs.dataoffsets, namely either:

  • a collection of states of the input iterator, corresponding to its positions at (i+j for j in SlidingDFTs.dataoffsets(method)),
  • or the fragment of data values from i to i+m where m = maximum(SlidingDFTs.dataoffsets(method)), plus the state at i+m - in order to update that fragment at the next iteration.

However, when I tried to implement that, I found no improvement in performance or memory allocations for the second kind of SDFTs, so I decided to keep it simple, and leave only the first kind, which works for both stateful and stateless input iterators. But I didn't change the interface, since I don't rule out the possibility of finding a smarter algorithm for stateless iterators, using the minimum amount of data about past data samples.

@roflmaostc
Copy link
Member

Thanks a lot! I try to look into it the next days :)

@roflmaostc
Copy link
Member

Looks good to me overall.
Just the question with the submodule and maybe changing the calling convention.

@heliosdrm
Copy link
Contributor Author

I have changed the way of creating the iterator. With the new design (ad33874), instead of sdft(SDFT(n), x) one should use SDFT(n)(x), as suggested. For this:

  • The function sdft (a wrapper of SDFTIterator) is removed.
  • A new abstract type AbstractSDFT is introduced, such that objects that subtype them can be called to create SDFTIterators.
  • SDFT is now a subtype of AbstractSDFT.
  • The documentation advises to create new SDFT implementation as subtypes of AbstractSDFT - of if not possible, make them explicitly callable to create SDFTIterators.

@heliosdrm
Copy link
Contributor Author

I have also sketched a commit removing the submodule SlidingDFTs, but not pushed it yet. That means introducing, among others, the following names in FourierTools: windowlength, dataoffsets, previousdata, nextdata, iterationcount.

They are rather generic names that might be useful for other purposes in FourierTools. I'll wait for instructions in order to see if I should either:
(a) retain the submodule to keep them in their own namespace
(b) rename the functions to fake a namespace without submodules (e.g. adding the sdft_ prefix).
(c) push if without renaming
(d) something else?

@roflmaostc
Copy link
Member

roflmaostc commented Jan 7, 2025

Maybe prefix the sdft_, then its good to me.

Anyway, we do not export those. So it's internal API.

If you can push the final changes I double check and we can merge :)

The following names are also slightly changed:
backdft > sdft_backindices
iterationcount > sdft_iteration
updatedft! > sdft_update!
@heliosdrm
Copy link
Contributor Author

heliosdrm commented Jan 7, 2025

Pushed with sdft_ prefix!
There are also a few changes in the nomenclature, commented in the commit, to make the new names less ugly. But the naming of the those functions is the part that I'm least fond of, so suggestions to improve it are welcome!

## Basic SDFT

"""
SDFT(n)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we maybe add here a minimal example with a iteration to see how it works?
Otherwise we can merge!

@roflmaostc
Copy link
Member

Let's merge?

@heliosdrm
Copy link
Contributor Author

Yes! (Though CI must run succesfully first, right?)

@roflmaostc
Copy link
Member

Running now :)

@codecov
Copy link

codecov bot commented Jan 8, 2025

Codecov Report

Attention: Patch coverage is 94.53125% with 7 lines in your changes missing coverage. Please review.

Project coverage is 94.03%. Comparing base (938be0f) to head (70d1bfc).
Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
src/sdft_interface.jl 94.73% 6 Missing ⚠️
src/sdft_implementations.jl 92.85% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #49      +/-   ##
==========================================
+ Coverage   93.97%   94.03%   +0.06%     
==========================================
  Files          17       19       +2     
  Lines        1045     1173     +128     
==========================================
+ Hits          982     1103     +121     
- Misses         63       70       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@roflmaostc
Copy link
Member

@heliosdrm
Copy link
Contributor Author

heliosdrm commented Jan 8, 2025

I've seen the codecov failures. They are due to not having implementations that need calling sdft_previousdft or more than one value from the previous data fragment yet. In the meanwhile I can define "test implementations" of the basic SDFT that use them (although those data are not necessary), so that they are used only for running the tests. I can prepare that this evening.

@heliosdrm
Copy link
Contributor Author

I hope that the added tests are sufficient to pass the coverage requirements.

@roflmaostc
Copy link
Member

Just some lines missing but maybe we can add them, then we are good to go

@heliosdrm
Copy link
Contributor Author

Just some lines missing but maybe we can add them, then we are good to go

ok, let's try now 🤞

@roflmaostc roflmaostc merged commit 32a2285 into bionanoimaging:main Jan 10, 2025
5 checks passed
@roflmaostc
Copy link
Member

Merged, thanks a lot!
I can do the release later and we can post it under our comments on discourse

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.

2 participants