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

post_process uses full correlation, fix filter_atoms #284

Merged
merged 4 commits into from
Aug 23, 2024

Conversation

harveydevereux
Copy link
Collaborator

@harveydevereux harveydevereux commented Aug 21, 2024

  • np.correlate(x, x, "full") otherwise we end up with correlations like so on the right, with the left being the output of the (forthcoming) online VAF calculation, see Adds Velocity, ShearStress, StressDiagonal, observables #285 for more details.
  • filter_atoms is changed since when I submit atom sequences like ((0, 2, 4, ...), (1, 3, 5, ...)) as is the case in the NaCl struct it converts it to (range(len(atoms)), (1, 3, 5, ...)) since 0 and 0 === False.

vaf

oerc0122
oerc0122 previously approved these changes Aug 21, 2024
janus_core/helpers/post_process.py Outdated Show resolved Hide resolved
oerc0122
oerc0122 previously approved these changes Aug 21, 2024
Copy link
Member

@ElliottKasoar ElliottKasoar left a comment

Choose a reason for hiding this comment

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

Can you add a very quick test for a VAF value, as presumably we don't currently if none of the tests broke? Maybe even just modify an existing test where we do post processing, so it could be very light weight?

Also could you clarify the difference between "same" and "full" in this context? Am I right in thinking we could recover "same" from "full", if we wanted? Would we ever want to?

@harveydevereux
Copy link
Collaborator Author

Can you add a very quick test for a VAF value, as presumably we don't currently if none of the tests broke? Maybe even just modify an existing test where we do post processing, so it could be very light weight?

Also could you clarify the difference between "same" and "full" in this context? Am I right in thinking we could recover "same" from "full", if we wanted? Would we ever want to?

Yes I'll ad a VAF test in there for the values.

On the same and full. Essentially it is due to how the signals are convolved. Full will perform a "full" convolution padding the edges of the array with 0's to do it. "same" will not. And so will be a subset, the "middle" of the full convolution.

Not sure if we'd want to use that for correlations honestly.

Also see the docs for np.convolve which np.correlate refers us to.

["full"] Note how the convolution operator flips the second array before “sliding” the two across one another:

["same"] Only return the middle values of the convolution. Contains boundary effects, where zeros are taken into account:


In detail, "full" will give you this (but twice over, symmetric about the middle value of the array)

$$ c_{n} = \sum_{i=0}^{N-n-1}x(i)\cdot y(i+n) $$

which is what the multi-tau correlator will also return (there is a factor of $\frac{1}{N-n}$ to correctly account for the averaging).

Same will do the same, but only up to the input size. The result is not a "full" correlation. It is the middle of it.

import numpy as np

from typing import Iterable

def correlate(
    x: Iterable[float], y: Iterable[float]) -> Iterable[float]:
    """Direct correlation of x and y."""
    n = min(len(x), len(y))
    cor = np.zeros(n)
    for j in range(n):
        for i in range(n - j):
            cor[j] += x[i] * y[i + j]
    return cor 

n = 5
a = [i for i in range(n)]

same = np.correlate(a, a, 'same')
full = np.correlate(a, a, 'full')[n-1:]
print("same", same)
print("full", full)
print("direct", correlate(a, a))
# same [11 20 30 20 11]
# full [ 0  4 11 20 30 20 11  4  0]
# direct [30. 20. 11.  4.  0.]

Going out to bigger inputs (n=64) it is more obvious

same [26288 27808 29359 30940 32550 34188 35853 37544 39260 41000 42763 44548
 46354 48180 50025 51888 53768 55664 57575 59500 61438 63388 65349 67320
 69300 71288 73283 75284 77290 79300 81313 83328 85344 83328 81313 79300
 77290 75284 73283 71288 69300 67320 65349 63388 61438 59500 57575 55664
 53768 51888 50025 48180 46354 44548 42763 41000 39260 37544 35853 34188
 32550 30940 29359 27808]
full [    0    63   188   374   620   925  1288  1708  2184  2715  3300  3938
  4628  5369  6160  7000  7888  8823  9804 10830 11900 13013 14168 15364
 16600 17875 19188 20538 21924 23345 24800 **26288** 27808 29359 30940 32550
 34188 35853 37544 39260 41000 42763 44548 46354 48180 50025 51888 53768
 55664 57575 59500 61438 63388 65349 67320 69300 71288 73283 75284 77290
 79300 81313 83328 85344 83328 81313 79300 77290 75284 73283 71288 69300
 67320 65349 63388 61438 59500 57575 55664 53768 51888 50025 48180 46354
 44548 42763 41000 39260 37544 35853 34188 32550 30940 29359 27808 **26288**
 24800 23345 21924 20538 19188 17875 16600 15364 14168 13013 11900 10830
  9804  8823  7888  7000  6160  5369  4628  3938  3300  2715  2184  1708
  1288   925   620   374   188    63     0]

Copy link
Member

@ElliottKasoar ElliottKasoar left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation, looks good!

@ElliottKasoar ElliottKasoar merged commit 8175ce7 into stfc:main Aug 23, 2024
8 checks passed
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.

3 participants