Skip to content

Conversation

s-kuberski
Copy link
Collaborator

This pull request addresses #256:

It removes the possibility to create an Obs from data that is based on several ensembles. Whereas the feature is needed internally, it can be considered dangerous for users as the behavior of the function might be unexpected. To allow for internal creation of such Obs in intermediate steps, the creation is still allowed if means are passed to the function.

Further changes that I introduced along the way because they are coupled to the same problem. It is possible that I have overlooked something. In this case, these changes can be discussed and reverted.

  • It is not possible anymore to reweight an Obs that already contains several ensembles. The only setting where I see that this could be a valid procedure is the case where algorithmic parameters that do not affect the reweighting but only the autocorrelation time have been changed such that two independent streams are not real replica and instead treated as separate ensembles. Also in this case, I would propose to force the user to reweight before combining the observables.
  • It is not possible anymore to correlate two observables that are based on several ensembles. I think that this is a feature that has to be applied to primary observables instead of derived observables, because otherwise averages are not properly defined.
  • The function merge_obs has been adapted to the same rules. The function header is extended to better explain what is done here.

Apart form this, the biggest changes in the code are in the JSON and dobs I/O, where I had to adapt to the new way to create the Obs and in the tests, where we had used the illegal creation in some cases.

It would be an option to add a routine that combines observables from several pseudo-replica by averaging them, either weighted by the number of configs or by the number of independent configs.

Copy link
Owner

@fjosw fjosw left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@fjosw
Copy link
Owner

fjosw commented Feb 19, 2025

Maybe @JanNeuendorf can confirm that this actually addresses the issue he raised.

@JanNeuendorf
Copy link
Collaborator

I think that it solves the issue because I see no remaining way to get a multi-ensemble Obs without taking an explicit average.

@s-kuberski
Copy link
Collaborator Author

By extending the tests to less trivial cases in some cases, I've discovered a small bug that I had introduced before. This is now fixed and I believe that all changes are now covered by the tests.

@s-kuberski s-kuberski marked this pull request as ready for review February 24, 2025 11:57
@fjosw fjosw merged commit 1779241 into fjosw:develop Feb 25, 2025
11 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