Fix inconsistent handling of figure names (backport #1430) #1434
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![mergify[bot]](https://avatars.githubusercontent.com/in/10562?s=60&v=4){kind=small}
`ExperimentData._clear_results()` cleared `ExperimentData._figures` but not the related `ExperimentData._db_data.figure_names` exposed as `ExperimentData.figure_names`. `BaseAnalysis.run()` calls `ExperimentData._clear_results()` so when a second analysis run produced a different set of figure names from the first the `ExperimentData` instance would be left a `figure_names` property with entries that did not match the underlying figures data. Here, this issue is addressed by also clearing `ExperimentData._db_data.figure_names`. This issue was first noticed due to occasional failures of the `test.database_service.test_db_experiment_data.TestDbExperimentData.test_copy_figure_artifacts` test which runs a fake experiment and then adds a figure and artifact to the experiment data. Then the tests copies the experiment data to test the behavior of `copy()`. Occasionally, the adding of the figure occurs before the background analysis thread runs. In this case, the figure name gets added to `figure_names` before the results get cleared by the analysis callback. Because of the bug described above, the old figure name stays around in the original experiment data object but does not copy to the new object because figure names are copied using the data in `ExperimentData._figures` and not `ExperimentData._db_data.figure_names`. A `block_for_results()` was added to the experiment because in principle the `copy()` could occur before the analysis clears results and then the two data objects would not match, though this case has not been observed (only adding the figure and then clearing it with analysis before copying so the copy is the one missing the figure name has been observed; not the copy having the name and not the original). Addtionally, `BaseAnalysis.run()` handled the `figure_names` option incorrectly when passes a keyword argument. Keyword arguments to passed to `run()` cause the analysis class to copy itself and add the options to the copy before running it. However, within the `run()` method, `self.options.figure_names` was used for assigning figure names rather than referencing the copy of the analysis class, so figure names could not be set using `figure_names` as a keyword argument. This issues was fixed by replacing `self` references with references to the copy. A regression test was added that fails for either of the above issues being present (figure names not matching after re-running analysis; figure names not settable from a `run()` keyword argument). (cherry picked from commit c4e9a52)
coruscating
approved these changes
Mar 31, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ExperimentData._clear_results()clearedExperimentData._figuresbut not the relatedExperimentData._db_data.figure_namesexposed asExperimentData.figure_names.BaseAnalysis.run()callsExperimentData._clear_results()so when a second analysis run produced a different set of figure names from the first theExperimentDatainstance would be left afigure_namesproperty with entries that did not match the underlying figures data. Here, this issue is addressed by also clearingExperimentData._db_data.figure_names.This issue was first noticed due to occasional failures of the
test.database_service.test_db_experiment_data.TestDbExperimentData.test_copy_figure_artifactstest which runs a fake experiment and then adds a figure and artifact to the experiment data. Then the tests copies the experiment data to test the behavior ofcopy(). Occasionally, the adding of the figure occurs before the background analysis thread runs. In this case, the figure name gets added tofigure_namesbefore the results get cleared by the analysis callback. Because of the bug described above, the old figure name stays around in the original experiment data object but does not copy to the new object because figure names are copied using the data inExperimentData._figuresand notExperimentData._db_data.figure_names. Ablock_for_results()was added to the experiment because in principle thecopy()could occur before the analysis clears results and then the two data objects would not match, though this case has not been observed (only adding the figure and then clearing it with analysis before copying so the copy is the one missing the figure name has been observed; not the copy having the name and not the original).Addtionally,
BaseAnalysis.run()handled thefigure_namesoption incorrectly when passes a keyword argument. Keyword arguments to passed torun()cause the analysis class to copy itself and add the options to the copy before running it. However, within therun()method,self.options.figure_nameswas used for assigning figure names rather than referencing the copy of the analysis class, so figure names could not be set usingfigure_namesas a keyword argument. This issues was fixed by replacingselfreferences with references to the copy.A regression test was added that fails for either of the above issues being present (figure names not matching after re-running analysis; figure names not settable from a
run()keyword argument).This is an automatic backport of pull request #1430 done by Mergify.