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

Fix/gaps #169

Merged
merged 11 commits into from
Apr 20, 2023
Merged

Fix/gaps #169

merged 11 commits into from
Apr 20, 2023

Conversation

s-kuberski
Copy link
Collaborator

I increased the flexibility of pyerrors to be able to cope with arbitrary combinations of gapped and irregular Monte Carlo chains among the different replica within one observable.

This was triggered by the observation that in the case where we work with multiple replica, a striding != 1 and have irregularities in one of the Monte Carlo chains, the code refused to perform the error propagation, e.g., in the case

dat = [np.random.normal(loc=1., size=10) for i in range(2)]
idl = [[0, 2, 4, 8, 10, 12, 14, 16, 18, 20], np.arange(0, 20, 2)]
o = pe.Obs(dat, ['A|r1', 'A|r2'], idl=idl)
o.gm()

This can be inconvenient in production. As a next step, I thought that, if one generalizes the code to cover this case, it should not matter at all, if we measured different stridings on different replica.

In order to integrate this change, I did modifications in gamma_method. Before, deltas were not expanded when the idl was a range (a regular chain) and it was expanded to a chain with striding one if it was a list (i.e., an irregular chain). Now, everything is expanded to chains where the striding is the minimal distance between two configs in the observable (the only exception being the case where we have regular chains with identical stridings. Here, no expansion has to be performed).
This change in the expansion paradigm led to necessary changes in the computation of the dvalue etc. (basicall I could revert everything that was inserted when the irregular chains where introduced).

I hope that the changes are coherent and that no error has been introduced. I tried to add some further tests, but this could possibly be expanded. Please check all the changes carefully. We should be sure that everything is absolutely correct, before merging this.

@s-kuberski s-kuberski added the enhancement New feature or request label Apr 5, 2023
@s-kuberski s-kuberski requested a review from fjosw as a code owner April 5, 2023 13:29
@fjosw
Copy link
Owner

fjosw commented Apr 6, 2023

Thanks, I think this is a useful feature. I created the following test to check the consistency of the gamma_method :

import numpy as np
import pyerrors as pe

dat = np.sin(np.arange(100) / 100)
for idl in [np.arange(100), np.arange(0, 1000, 10)]:
    my_obs = pe.Obs([dat], ["test_ens"], idl=[idl])
    assert np.isclose(my_obs.value, 0.4554865083873183)

    my_obs.gm(S=0)
    assert np.isclose(my_obs.dvalue, 0.02495954189079061)
    my_obs.gm()
    assert np.isclose(my_obs.dvalue, 0.11817931680985193)

The test passes on develop but fails with your changes as the window selection seems to have changed. It would be good to understand why that is the case.

@s-kuberski
Copy link
Collaborator Author

Thanks die looking into this, I’ll investigate the behavior.

@s-kuberski s-kuberski marked this pull request as draft April 6, 2023 18:59
@s-kuberski
Copy link
Collaborator Author

I found the bug and fixed it, but there are some other issues that may arise if the requirement is lifted that the striding has to be the same for all replica. We do not have to necessarily do this as it might be enough to resolve the issue with the mix of regular and irregular chains with common striding > 1. I'll try to identify and address further edge cases before updating this pull request.

@s-kuberski
Copy link
Collaborator Author

I made my mind up concerning replica with differing regular distances between configurations (possibly including gaps):

It should now be possible to combine such chains if the minimal distance between two configurations is a common divisor of all involved distances. I.e., it is possible to combine replica with gaps (1, 2, 4) or with (3, 6) but not with (2, 3). In the latter case, the windowing procedure would end up in undefined behaviour because rho has entries that are zero. The autocorrelation analysis is now done in terms of this minimal distance between two configurations.

I did a number of tests and also extended the automated tests in the package. Everything looks fine, so far, but additional tests might be required. I will try to create more of them.

I did not (yet?) update the documentation because we did not say anything about the old requirement that the gap sizes had to match among the different replica. Maybe it is fine to leave it like this because I would not really want to suggest that it is a good idea to wildly mix the measurement strategy for different replica. My main concern was the error propagation with gap sizes != 1 and irregular chains that works in the new implementation.

@fjosw
Copy link
Owner

fjosw commented Apr 20, 2023

Is this ready for review or do you still want to continue testing?

@s-kuberski
Copy link
Collaborator Author

I did not (yet?) find any further issues of the current status of the implementation. It could be helpful if you have another look at this.

@s-kuberski s-kuberski marked this pull request as ready for review April 20, 2023 12:45
@fjosw
Copy link
Owner

fjosw commented Apr 20, 2023

I went over the changes and did not spot any problems. I will merge this and then we can do a bit more testing on develop before the next release.

@fjosw fjosw merged commit a214465 into fjosw:develop Apr 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants