Skip to content

Conversation

elindgren
Copy link
Collaborator

@elindgren elindgren commented Apr 7, 2025

Adds a weighting parameter "weight" to an Experiment.
The residuals are weighted by a weighting factor. Note that this means that the contribution to chi_squared is weighted by a factor of weight**2.

Skipping error handling for now, ideally the joint_fit object should be converted to a Python object that validates the weights etc. But let's not get ahead of ourselves.

Fixes #5

@elindgren
Copy link
Collaborator Author

Migrating some of the discussion here for future reference:

By @AndrewSazonov

I’d suggest the following API structure, following the same logic we use for linked phases. For example:

expt.linked_phases.add("phase1", scale=0.5)
expt.linked_phases.add("phase2", scale=1.3)

In this model, the phase doesn’t know anything about its contribution to an experiment. It’s the experiment that tracks which phases are involved, and their relative contributions (via scale).

For experiments, I propose we follow the same principle. An experiment is just an experiment. It shouldn't know anything about its role or weight in a fitting process. That responsibility should live in the analysis object.

Suppose we have:

analysis.fit_mode = "joint"

When this mode is selected, a collection like analysis.joint_fit becomes accessible. Then we can do:

analysis.joint_fit.add("expt1", weight=0.4)  # Default weight could be 0.5
analysis.joint_fit.add("expt2", weight=0.6)  # Default weight could be 0.5

Only the experiments listed in joint_fit would be used during analysis.fit().

Alternatively, switching analysis.fit_mode from "single" to "joint" could automatically populate analysis.joint_fit with all defined experiments, each with equal default weights.

Then the user can adjust weights like this:

analysis.joint_fit["expt1"].weight = 0.4
analysis.joint_fit["expt2"].weight = 0.6

This keeps the model consistent with how we've already handled phase contributions and keeps responsibility for fit-specific configuration clearly within the analysis object.

@elindgren
Copy link
Collaborator Author

By @elindgren

@AndrewSazonov I decided on keeping the weights in the _residual_function, instead of moving them to where reduced chi-squared is computed. It didn't really make sense to have it there, especially when considering other fitting modes than joint. Instead, I multiply the residuals with the square roots of the weights, so that the weights enter linearly into the chi-squared.

Also, I normalized the weights such that they sum up to the number of experiments. A two-experiment fit, with weights 0.5, will thus effectively have weights 1.0 and leave the reduced chi-squared unchanged compared to before. Thus, if one adds multiple experiments, one would expect the chi-squared to grow with the number of expeirments, even if all fits are good. Alternatively, one could have the weights sum up to 1, which would leave the chi-squared unchanged as more experiments are added (if all the fits are equally good). I'm not really sure which of these two is preferred, what do you say @AndrewSazonov?

For reference, we decided that the sum of weights should sum up to the number of experiments. This ensures that one retrieves the same reduced chi_squared when fitting a dataset as when splitting the dataset in two parts and fitting the two parts together.

@elindgren elindgren added the [scope] enhancement Adds/improves features (major.MINOR.patch) label Apr 7, 2025
@elindgren elindgren requested a review from AndrewSazonov April 7, 2025 11:33

# Compare fit quality
assert_almost_equal(project.analysis.fit_results.reduced_chi_square, 21.1, decimal=1)
assert_almost_equal(project.analysis.fit_results.reduced_chi_square, 14.4, decimal=1)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that I had to adjust the expected chi_squared after changing the target sum of weights.


# Run the fitting process
experiment_ids = list(experiments._items.keys())

Copy link
Member

Choose a reason for hiding this comment

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

the line above should also reuse the new experiments.ids property

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed, thanks!

@AndrewSazonov
Copy link
Member

Two comments for now:

  1. Changing the weights of the two experiments in joint-fit_split-single-dataset.py doesn’t seem to affect the resulting chi2 value - it consistently remains at 4.66. This value should vary depending on the individual experiment weights.

  2. I suggest implementing the following API for selecting and weighting experiments in a joint fit:

    project.analysis.joint_fit_experiments.add("npd", weight=0.6)
    project.analysis.joint_fit_experiments.add("xrd", weight=0.4)

    And to update the weights later, if needed:

    project.analysis.joint_fit_experiments["npd"].weight = 0.7
    project.analysis.joint_fit_experiments["xrd"].weight = 0.3

@elindgren
Copy link
Collaborator Author

@AndrewSazonov I've implemented the two points we discussed yesterday.

@AndrewSazonov
Copy link
Member

It works as expected in terms of the resulting chi2 values. Thank you @elindgren for implementing this.

One minor API detail. The correct way to update experiment weights should be:

project.analysis.joint_fit_experiments["npd"].weight = 0.7
project.analysis.joint_fit_experiments["xrd"].weight = 0.3

instead of:

project.analysis.joint_fit_experiments["npd"] = 0.7
project.analysis.joint_fit_experiments["xrd"] = 0.3

But, since I’m currently working on significant changes in the fix-output branch, I suggest we go ahead and merge this PR, and I’ll handle the adjustment there.

@elindgren
Copy link
Collaborator Author

It works as expected in terms of the resulting chi2 values. Thank you @elindgren for implementing this.

One minor API detail. The correct way to update experiment weights should be:

project.analysis.joint_fit_experiments["npd"].weight = 0.7
project.analysis.joint_fit_experiments["xrd"].weight = 0.3

instead of:

project.analysis.joint_fit_experiments["npd"] = 0.7
project.analysis.joint_fit_experiments["xrd"] = 0.3

But, since I’m currently working on significant changes in the fix-output branch, I suggest we go ahead and merge this PR, and I’ll handle the adjustment there.

ah okay, I missed that! Sounds good, I'll merge then

@elindgren elindgren merged commit 48f365c into develop Apr 9, 2025
@AndrewSazonov AndrewSazonov deleted the weight-experiments branch August 6, 2025 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[scope] enhancement Adds/improves features (major.MINOR.patch)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants