Skip to content

Commit 29e8a30

Browse files
committed
CR review issues addressed
1 parent ab4ad47 commit 29e8a30

17 files changed

Lines changed: 358 additions & 113 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,12 +22,16 @@
2222
- Each experiment's model is chosen per row on the Experiment and Analysis
2323
pages and shows the stored pairing (the combo used to reset to the first
2424
model and could change another experiment).
25-
- A "Fit" checkbox per experiment chooses what the next fit includes; a
26-
parameter that an included model follows stays fitted even when its own
27-
model is left out. The results dialog lists each dataset's χ² per point
28-
and share of the total; the statistics come from the library's record of
29-
the run, so they survive switching the current model.
30-
- The Analysis chart has the staggered view too.
25+
- A "Fit" checkbox per experiment chooses what the next fit, or Bayesian
26+
sampling, includes; a parameter that an included model follows stays
27+
fitted even when its own model is left out. The pre-fit bound checks look
28+
only at the parameters the fit varies. The results dialog lists each
29+
dataset's χ² per point and share of the total; the statistics come from
30+
the library's record of the last completed run, so they survive switching
31+
the current model and are not replaced by a failed or cancelled run.
32+
- The Analysis chart has the staggered view too (not in the R·q⁴ view).
33+
- Linking several models links all of them or, when one cannot be linked,
34+
none.
3135
- Removing a model that experiments use asks whether to remove them or fit
3236
them with another model; experiments are no longer removed by position.
3337
- A data file that cannot be read is reported in a dialog.

‎EasyReflectometryApp/Backends/Py/analysis.py‎

Lines changed: 15 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -564,11 +564,9 @@ def _on_fit_failed(self, error_message: str) -> None:
564564
is_user_cancel = self._fitting_logic.fit_cancelled and 'cancel' in error_message.lower()
565565
if is_user_cancel:
566566
error_message = 'Fitting cancelled by user'
567+
# Not recorded on the project: its record keeps describing the last completed fit,
568+
# which the parameters still hold. The dialog shows this error.
567569
self._fitting_logic.on_fit_failed(error_message)
568-
try:
569-
self._fitting_logic.record_on_project(None, 'cancelled' if is_user_cancel else 'failed')
570-
except Exception:
571-
logger.exception('Failed to record the fit on the project')
572570
self._fitter_thread = None
573571
self.fittingChanged.emit()
574572
self._clearCacheAndEmitParametersChanged()
@@ -1048,15 +1046,19 @@ def _prefit_errors(self) -> List[str]:
10481046
return errors
10491047

10501048
def _free_parameters(self) -> List[dict]:
1051-
"""The parameters a fit varies, each once.
1049+
"""The parameters the next fit varies, each once.
10521050
10531051
Read from the unfiltered list: the table's name and free/fixed filters
10541052
must not change what the fit-wide checks see. A parameter shared by
1055-
several models is listed once per model, hence the identity check.
1053+
several models is listed once per model, hence the identity check. The
1054+
library says which parameters the included experiments' fit varies
1055+
(``Project.fit_parameters``), so a model left out does not block it.
10561056
"""
1057+
fit_parameters = getattr(self._project_lib, 'fit_parameters', None)
1058+
in_fit = None if fit_parameters is None else {id(parameter) for parameter in fit_parameters()}
10571059
unique = {}
10581060
for param in self._parameters_logic.all_parameters():
1059-
if param['fit'] and param.get('enabled', True):
1061+
if param['fit'] and param.get('enabled', True) and (in_fit is None or id(param['object']) in in_fit):
10601062
unique.setdefault(id(param['object']), param)
10611063
return list(unique.values())
10621064

@@ -1168,12 +1170,7 @@ def removeExperiment(self, index: int) -> None:
11681170
Remove the experiment at the given index.
11691171
"""
11701172
if 0 <= index < len(self._experiments_logic.available()):
1171-
self._experiments_logic.remove_experiment(index)
1172-
# The selection follows: the removed experiment leaves it, later ones shift down.
1173-
if self._selection.remove_index(index, len(self._experiments_logic.available())):
1174-
self._sync_current_experiment_to_selection()
1175-
self.experimentsChanged.emit()
1176-
self.externalExperimentChanged.emit()
1173+
self.follow_rekeyed_experiments(self._experiments_logic.remove_experiment(index))
11771174
else:
11781175
logger.warning('Experiment index %s is out of range.', index)
11791176

@@ -1211,14 +1208,11 @@ def setSelectedExperimentIndices(self, indices: List[int]) -> None:
12111208
self.experimentsChanged.emit()
12121209
self.externalExperimentChanged.emit()
12131210

1214-
def follow_removed_experiments(self, removed: list) -> None:
1215-
"""The selection follows experiments removed elsewhere (with their model): the removed
1216-
ones leave it and the later ones move up, as for :meth:`removeExperiment`."""
1217-
remaining = len(self._experiments_logic.available())
1218-
changed = False
1219-
for index in sorted(removed, reverse=True):
1220-
changed |= self._selection.remove_index(index, remaining)
1221-
if changed:
1211+
def follow_rekeyed_experiments(self, keys: dict) -> None:
1212+
"""The selection follows removed experiments, by the library's old -> new key table
1213+
(the removed ones are not in it). The experiment list changed, so it is announced
1214+
whether or not the selection did."""
1215+
if self._selection.remap(keys, len(self._experiments_logic.available())):
12221216
self._sync_current_experiment_to_selection()
12231217
self.experimentsChanged.emit()
12241218
self.externalExperimentChanged.emit()

‎EasyReflectometryApp/Backends/Py/logic/experiment_selection.py‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -33,11 +33,10 @@ def set_indices(self, indices, available_count: int) -> bool:
3333
self._indices = valid
3434
return True
3535

36-
def remove_index(self, removed: int, available_count: int) -> bool:
37-
"""Follow the removal of experiment `removed`: drop it and shift the later ones down.
38-
`available_count` is the number of experiments left."""
39-
shifted = [index - 1 if index > removed else index for index in self._indices if index != removed]
40-
return self.set_indices(shifted, available_count)
36+
def remap(self, keys: dict, available_count: int) -> bool:
37+
"""Follow removed experiments with the library's old -> new key table (the removed
38+
ones are not in it). `available_count` is the number of experiments left."""
39+
return self.set_indices([keys[index] for index in self._indices if index in keys], available_count)
4140

4241
def prune(self, available_count: int) -> bool:
4342
"""Drop indices that no longer exist (after experiments were removed elsewhere)."""

‎EasyReflectometryApp/Backends/Py/logic/experiments.py‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -166,10 +166,11 @@ def set_included_in_fit(self, index: int, included: bool) -> bool:
166166
experiment.include_in_fit = included
167167
return True
168168

169-
def remove_experiment(self, index: int) -> None:
170-
"""Remove the experiment at the given (ordered) index; the library re-keys the rest."""
169+
def remove_experiment(self, index: int) -> dict:
170+
"""Remove the experiment at the given (ordered) index; returns the library's old -> new
171+
key table of the remaining ones (empty when nothing was removed)."""
171172
key = self._experiment_key_at_index(index)
172173
if key is None:
173174
logger.warning('Experiment index %s is out of range.', index)
174-
return
175-
self._project_lib.remove_experiment(key)
175+
return {}
176+
return self._project_lib.remove_experiment(key)

‎EasyReflectometryApp/Backends/Py/logic/fitting.py‎

Lines changed: 35 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,7 @@ def on_fit_failed(self, error_message: str) -> None:
187187
"""
188188
self._result = None
189189
self._results = []
190+
self._prepared = None
190191
self._fit_error_message = error_message
191192
self._running = False
192193
self._finished = True
@@ -248,11 +249,14 @@ def _ordered_experiments(self) -> list:
248249
'Bayesian sampling of polarized experiments is not supported yet.'
249250
)
250251

251-
def _has_polarized_experiments(self) -> bool:
252-
"""Whether any loaded experiment carries per-channel (polarized) data."""
253-
return any(
254-
getattr(experiment, 'available_channels', None) is not None for experiment in self._ordered_experiments()
255-
)
252+
def _has_polarized_experiments(self, experiments: Optional[list] = None) -> bool:
253+
"""Whether any of `experiments` (by default every loaded one) carries per-channel data."""
254+
experiments = self._ordered_experiments() if experiments is None else experiments
255+
return any(getattr(experiment, 'available_channels', None) is not None for experiment in experiments)
256+
257+
def _included_experiments(self) -> list:
258+
"""The experiments whose Fit box is ticked, in order."""
259+
return [experiment for experiment in self._ordered_experiments() if getattr(experiment, 'include_in_fit', True)]
256260

257261
# ------------------------------------------------------------------
258262
# Inequality constraints (BUMPS penalties)
@@ -377,11 +381,13 @@ def _fail_before_start(self, message: str) -> None:
377381
self._finished = True
378382
self._show_results_dialog = True
379383

380-
def record_on_project(self, results: Optional[list], status: str = 'completed') -> None:
381-
"""Record the outcome of the worker's run on the project (``Project.last_fit``),
382-
which the results dialog and the summary read."""
384+
def record_on_project(self, results: list) -> None:
385+
"""Record the completed run on the project (``Project.last_fit``), which the results
386+
dialog and the summary read. The prepared run is used up: a later failure, or a
387+
sampling run, cannot be recorded against it."""
383388
if self._prepared is not None:
384-
self._project_lib.record_fit(self._prepared, results, status)
389+
prepared, self._prepared = self._prepared, None
390+
self._project_lib.record_fit(prepared, results)
385391

386392
@property
387393
def _run(self):
@@ -399,6 +405,7 @@ def fit_dataset_rows(self) -> list[dict]:
399405
{
400406
'name': name if channel is None else f'{name} ({channel})',
401407
'points': entry['objective_n_points'],
408+
'measuredPoints': entry['classical_n_points'],
402409
'chi2': entry['objective_chi2'],
403410
'chi2PerPoint': entry['objective_chi2_per_point'],
404411
'share': entry['share_of_objective'],
@@ -430,25 +437,23 @@ def fit_classical_reduced_chi2(self) -> float | None:
430437
# Bayesian sampling helpers
431438
# ------------------------------------------------------------------
432439

433-
def collect_all_experiments_datagroup(self) -> 'sc.DataGroup':
440+
def collect_all_experiments_datagroup(self, experiments: Optional[list] = None) -> 'sc.DataGroup':
434441
"""Build the scipp DataGroup required by reflectometry-lib ``MultiFitter.mcmc_sample()``.
435442
436-
Scope decision (see issue #319): Bayesian sampling deliberately runs over
437-
**all** experiments, mirroring the classical fit path (``prepare_threaded_fit``)
438-
which also fits every experiment. The experiment selection currently affects
439-
only plotting, not the fit/sampling scope. If per-selection sampling is ever
440-
wanted, filter ``self._ordered_experiments()`` here and rename accordingly.
443+
Sampling covers the experiments whose Fit box is ticked, like a classical fit;
444+
the selection only affects plotting.
441445
446+
:param experiments: The experiments to sample, by default the included ones.
442447
:return: DataGroup with reflectivity coords and data.
443448
:rtype: sc.DataGroup
444449
"""
445450
import numpy as np
446451
import scipp as sc
447452

448-
if self._has_polarized_experiments():
453+
experiments = self._included_experiments() if experiments is None else experiments
454+
if self._has_polarized_experiments(experiments):
449455
raise ValueError(self._POLARIZED_SAMPLE_MESSAGE)
450456

451-
experiments = self._ordered_experiments()
452457
coords = {}
453458
data = {}
454459
for i, experiment in enumerate(experiments):
@@ -483,17 +488,26 @@ def prepare_threaded_sample(self, minimizers_logic: 'Minimizers') -> tuple:
483488
try:
484489
from easyreflectometry.fitting import MultiFitter
485490

486-
experiments = self._ordered_experiments()
487-
if not experiments:
491+
if not self._ordered_experiments():
488492
self._fail_before_start('No experiments to sample')
489493
return None, None
494+
experiments = self._included_experiments()
495+
if not experiments:
496+
self._fail_before_start('No experiment to sample: tick Fit for at least one experiment.')
497+
return None, None
498+
unbound = [experiment.name for experiment in experiments if experiment.model is None]
499+
if unbound:
500+
names = ', '.join(f"'{name}'" for name in unbound)
501+
self._fail_before_start(f'{names} cannot be sampled: no model is assigned.')
502+
return None, None
490503

491504
constraints_error = self.inequality_constraints_error(minimizers_logic)
492505
if constraints_error:
493506
logger.warning('Sampling refused: %s', constraints_error)
494507
self._fail_before_start(constraints_error)
495508
return None, None
496509

510+
# The library adds the free parameters of excluded models that these follow.
497511
models = [experiment.model for experiment in experiments]
498512
multi_fitter = MultiFitter(*models)
499513
# Sampled with a snapshot of the project's settings (a BUMPS minimizer
@@ -502,7 +516,7 @@ def prepare_threaded_sample(self, minimizers_logic: 'Minimizers') -> tuple:
502516
# controls stay editable meanwhile.
503517
multi_fitter.settings = copy.deepcopy(self._project_lib.fit_settings)
504518

505-
data_group = self.collect_all_experiments_datagroup()
519+
data_group = self.collect_all_experiments_datagroup(experiments)
506520
return multi_fitter, data_group
507521
except Exception as e:
508522
self._fail_before_start(f'Error preparing sampling: {e}')
@@ -511,6 +525,7 @@ def prepare_threaded_sample(self, minimizers_logic: 'Minimizers') -> tuple:
511525

512526
def prepare_for_threaded_sample(self) -> None:
513527
"""Set running flags and sampling progress message before launching the worker."""
528+
self._prepared = None # a sampling run is not recorded as a classical one
514529
self.reset_stop_flag()
515530
self._running = True
516531
self._finished = False

‎EasyReflectometryApp/Backends/Py/logic/models.py‎

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
from easyreflectometry.model import Model
99
from easyreflectometry.model import ModelCollection
1010
from easyreflectometry.model.resolution_functions import PercentageFwhm
11+
from easyreflectometry.project import ModelRemoval
1112

1213
from .helpers import get_original_name
1314

@@ -19,7 +20,7 @@ def _formula_of(target) -> str:
1920
class Models:
2021
def __init__(self, project_lib: ProjectLib):
2122
self._project_lib = project_lib
22-
self._contrast_candidates: list = []
23+
self._contrast_candidates: tuple = (None, [])
2324

2425
@property
2526
def _models(self) -> ModelCollection:
@@ -98,7 +99,7 @@ def contrast_candidates(self, index: int) -> list[dict]:
9899
(``'material'``: replace it by another material; ``'formula'``: give it another
99100
chemical formula) and, for a formula, the current one."""
100101
materials, formulas = substitution_candidates(self._project_lib.models[index])
101-
self._contrast_candidates = materials + formulas
102+
self._contrast_candidates = (self._project_lib.models[index], materials + formulas)
102103
rows = [{'label': material.name, 'kind': 'material'} for material in materials]
103104
rows += [{'label': target.name, 'kind': 'formula', 'formula': _formula_of(target)} for target in formulas]
104105
# Two distinct objects may share a name (each surfactant layer has its own D2O).
@@ -113,10 +114,18 @@ def contrast_candidates(self, index: int) -> list[dict]:
113114
def add_contrast(self, index: int, name: str, choices: list[dict]) -> int:
114115
"""Add a contrast of the model at `index`; `choices` refer to the rows of the last
115116
:meth:`contrast_candidates`, each with a palette ``material`` index or a ``formula``.
116-
Returns the new model's index."""
117+
Returns the new model's index.
118+
119+
Raises ValueError when the model changed since the candidates were listed: a row
120+
would then name another object."""
121+
model, candidates = self._contrast_candidates
122+
materials, formulas = substitution_candidates(self._project_lib.models[index])
123+
current = materials + formulas
124+
if model is not self._project_lib.models[index] or [id(c) for c in current] != [id(c) for c in candidates]:
125+
raise ValueError('The model changed since the dialog opened; open it again.')
117126
substitutions = []
118127
for choice in choices:
119-
candidate = self._contrast_candidates[int(choice['candidate'])]
128+
candidate = candidates[int(choice['candidate'])]
120129
if 'material' in choice:
121130
material = self._project_lib._materials[int(choice['material'])]
122131
if material is not candidate:
@@ -130,14 +139,14 @@ def experiments_using(self, index: int) -> list[str]:
130139
experiments = self._project_lib.experiments
131140
return [experiments[key].name for key in self._project_lib.experiments_for_model(index)]
132141

133-
def remove_at_index(self, index: int, rebind_to: int = -1) -> list[int]:
142+
def remove_at_index(self, index: int, rebind_to: int = -1) -> ModelRemoval:
134143
"""Remove the model; its experiments are removed, or bound to `rebind_to` when that is
135-
a model index. Returns the keys of the experiments removed with it."""
144+
a model index (counted before the removal). Returns the library's account of it: with
145+
the experiments removed, ``remap`` keys the remaining ones."""
136146
experiments = None
137147
if self._project_lib.experiments_for_model(index):
138148
experiments = rebind_to if rebind_to >= 0 else 'remove'
139-
removal = self._project_lib.remove_model_at_index(index, experiments=experiments)
140-
return removal.experiments if experiments == 'remove' else []
149+
return self._project_lib.remove_model_at_index(index, experiments=experiments)
141150

142151
def default_model_content(self, model: Model) -> None:
143152
"""Set the default content for a model."""
@@ -189,15 +198,14 @@ def _attach_calculator(self, model: Model) -> None:
189198
"""
190199
model.interface = self._project_lib._calculator
191200

201+
# The library keeps the moved model the current one.
192202
def move_selected_up(self) -> None:
193203
if self.index > 0:
194204
self._project_lib.move_model(self.index, self.index - 1)
195-
self.index = self.index - 1
196205

197206
def move_selected_down(self) -> None:
198207
if self.index < len(self._models) - 1:
199208
self._project_lib.move_model(self.index, self.index + 1)
200-
self.index = self.index + 1
201209

202210

203211
def _from_models_collection_to_list_of_dicts(models_collection: ModelCollection) -> list[dict[str, str]]:

‎EasyReflectometryApp/Backends/Py/py_backend.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -296,7 +296,7 @@ def _connect_project_page(self) -> None:
296296
def _connect_sample_page(self) -> None:
297297
# Removing a model can remove its experiments. Connected first, so that every
298298
# handler below already sees a selection without them.
299-
self._sample.experimentsRemoved.connect(self._analysis.follow_removed_experiments)
299+
self._sample.experimentsRekeyed.connect(self._analysis.follow_rekeyed_experiments)
300300
self._sample.modelsTableChanged.connect(self._analysis.prune_selected_experiments)
301301
self._sample.externalSampleChanged.connect(self._relay_sample_page_sample_changed)
302302
# Enabling magnetism can switch the project's calculation engine; the

0 commit comments

Comments
 (0)