-
Notifications
You must be signed in to change notification settings - Fork 23
Deprecate and refactor: Deprecate and refactor initializeResults and resultsDictionary
#161
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
base: v3.3.0
Are you sure you want to change the base?
Changes from 7 commits
8a3c58f
47725c6
246392c
031a0d5
033318d
8375287
138422f
c1c4af8
45860b1
42ec994
e585f2f
37adb66
24acc55
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| **Added:** | ||
|
|
||
| * Added ``get_results_dictionary`` method to ``FitResults`` to with improved parsing abilities. | ||
| * Added ``initialize_recipe_from_results`` method to ``FitRecipe`` to initialize a recipe from a ``FitResults`` object or text file. | ||
|
|
||
| **Changed:** | ||
|
|
||
| * <news item> | ||
|
|
||
| **Deprecated:** | ||
|
|
||
| * Deprecated ``initializeResults`` for removal in 4.0.0. Use ``FitRecipe.initialize_recipe_from_results`` instead. | ||
| * Deprecated ``resultsDictionary`` for removal in 4.0.0. Use ``FitResults.get_results_dictionary`` instead. | ||
|
|
||
| **Removed:** | ||
|
|
||
| * <news item> | ||
|
|
||
| **Fixed:** | ||
|
|
||
| * <news item> | ||
|
|
||
| **Security:** | ||
|
|
||
| * <news item> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,6 +35,7 @@ | |
| __all__ = ["FitRecipe"] | ||
|
|
||
| from collections import OrderedDict | ||
| from pathlib import Path | ||
|
|
||
| import matplotlib.pyplot as plt | ||
| from bg_mpl_stylesheets.styles import all_styles | ||
|
|
@@ -1476,5 +1477,47 @@ def _update_configuration(self): | |
| self._ready = False | ||
| return | ||
|
|
||
| def _load_results_file(self, results_path): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you remind me why we are creating the string then reading it rather than just loading the dict from the I get it that a user may have a results file and want to use it initialize a fit, so I see that this is useful, but in the context of a sequential refinement that we are running internally, it doesn't make too much sense to me because we have the Recipe. But I remember we talked about this and you mentioned something sensible. I just don't remember. But this is causing me pain due to lack of elegance....
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sbillinge Initially, I did it this way because that is how If you think its worth it, we could make it such that when a results object or recipe object is passed through the initializer it get the values in an easier way (without parsing). But then it seems like extra code doing the same thing. FYI heres what a loop using the initializer might look like
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would put these on separate PRs, they are different UCs in my mind. For sequential refinement we want it to be super robust. So no results file anywhere in the picture. Build the parameter dict from the object. Then a separate UC which can be more buggy and difficult to maintain (it could break every time we correct a typo in the results file) that allows you to instantiate a Recipe object from a results file. Users might find this useful but if it breaks it is on them a bit if you see what I mean. this would then support the UC that the user has a set of results files and wants to recreate the refinement, but we get the parsing out of the sequential refinement making it way more robust. Tl;dr, let's leave this PR open and can harvest it for my second UC, but start a new PR that does sequential refinement in a more robust way without parsing the results file. YOu can also harvest code from here that does the sequential refinement from the parameter dict, just use different code to build that dict. That would be my take.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sbillinge okay good idea. I can get rolling on that. Then I'll turn this PR into the "initialize from results file" one
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. a better approach than parsing the results file could be to save the Fitrecipe object itself. We do this in PDFgui where we have the project files. these are pickles of the fit object. Here we could pickle the fit recipe and then reopen it. Pickling is considered somewhat unsafe because of the chance of executable code but there are safer ways of doing the "serialization". Let's make an issue to request this behavior that I think users would find super useful. Not to say we will do it right away, but a good project for someone at some point.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sbillinge okay made the issue #163 |
||
| """Load a saved FitResults file and return parsed values.""" | ||
| from diffpy.srfit.fitbase import FitResults | ||
|
|
||
| with open(results_path) as f: | ||
| text = f.read() | ||
| results_dict = FitResults._parse_results_text(text) | ||
| return results_dict | ||
|
|
||
| def _prepare_results_for_initialization(self, results): | ||
| from diffpy.srfit.fitbase import FitResults | ||
sbillinge marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| if isinstance(results, FitResults): | ||
| results_dict = results.get_results_dictionary() | ||
| elif isinstance(results, (str, Path)): | ||
| results_path = Path(results) | ||
| results_dict = self._load_results_file(results_path) | ||
| else: | ||
| raise ValueError( | ||
| "results must be a FitResults object, str, or pathlib.Path" | ||
sbillinge marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| ) | ||
| # Remove metrics like Rw from the dict, leaving only parameter values | ||
| parameters_dict = { | ||
| k: v for k, v in results_dict.items() if isinstance(v, tuple) | ||
| } | ||
sbillinge marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return parameters_dict | ||
|
|
||
| def initialize_recipe_from_results(self, results): | ||
| """Initialize the variable values from a previous fit result. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| results : FitResults object, str, or pathlib.Path object | ||
| A FitResults object from a previous fit. The variable values will | ||
| be taken from the 'x' attribute of the FitResults object, | ||
| which is the list of variable values at the best fit. | ||
| """ | ||
| parameters_dict = self._prepare_results_for_initialization(results) | ||
| for name, (value, uncertainty) in parameters_dict.items(): | ||
| if name in self._parameters: | ||
| self._parameters[name].setValue(value) | ||
|
|
||
|
|
||
| # End of file | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,7 +39,7 @@ | |
| formatResults_dep_msg = build_deprecation_message( | ||
| fitresults_base, | ||
| "formatResults", | ||
| "get_results_string", | ||
| "format_results_string", | ||
| removal_version, | ||
| ) | ||
|
|
||
|
|
@@ -57,6 +57,21 @@ | |
| removal_version, | ||
| ) | ||
|
|
||
| resultsDictionary_dep_msg = ( | ||
| "'diffpy.srfit.fitbase.fitresults.resultsDictionary' is deprecated " | ||
| "and will be removed in version 4.0.0. Please use " | ||
| "'diffpy.srfit.fitbase.FitResults.get_results_dictionary' " | ||
| "instead." | ||
| ) | ||
|
|
||
|
|
||
| initializeRecipe_dep_msg = ( | ||
| "'diffpy.srfit.fitbase.initializeRecipe' is deprecated " | ||
| "and will be removed in version 4.0.0. Please use " | ||
| "'diffpy.srfit.fitbase.FitRecipe.initialize_recipe_from_results' " | ||
| "instead." | ||
| ) | ||
|
|
||
|
|
||
| class FitResults(object): | ||
| """Class for processing, presenting and storing results of a fit. | ||
|
|
@@ -349,7 +364,7 @@ def _calculate_constraint_uncertainties(self): | |
| self.conunc.append(sig2c**0.5) | ||
| return | ||
|
|
||
| def get_results_string(self, header="", footer="", update=False): | ||
| def format_results_string(self, header="", footer="", update=False): | ||
| """Format the results and return them in a string. | ||
|
|
||
| This function is called by print_results and save_results. Overloading | ||
|
|
@@ -546,10 +561,10 @@ def formatResults(self, header="", footer="", update=False): | |
| """This function has been deprecated and will be removed in version | ||
| 4.0.0. | ||
|
|
||
| Please use diffpy.srfit.fitbase.FitResults.get_results_string | ||
| Please use diffpy.srfit.fitbase.FitResults.format_results_string | ||
| instead. | ||
| """ | ||
| return self.get_results_string(header, footer, update) | ||
| return self.format_results_string(header, footer, update) | ||
|
|
||
| def print_results(self, header="", footer="", update=False): | ||
| """Format and print the results. | ||
|
|
@@ -563,7 +578,7 @@ def print_results(self, header="", footer="", update=False): | |
| update | ||
| Flag indicating whether to call update() (default False). | ||
| """ | ||
| print(self.get_results_string(header, footer, update).rstrip()) | ||
| print(self.format_results_string(header, footer, update).rstrip()) | ||
| return | ||
|
|
||
| @deprecated(printResults_dep_msg) | ||
|
|
@@ -578,7 +593,7 @@ def printResults(self, header="", footer="", update=False): | |
| return | ||
|
|
||
| def __str__(self): | ||
| return self.get_results_string() | ||
| return self.format_results_string() | ||
|
|
||
| def save_results(self, filename, header="", footer="", update=False): | ||
| """Format and save the results. | ||
|
|
@@ -602,7 +617,7 @@ def save_results(self, filename, header="", footer="", update=False): | |
| myheader += "produced by " + getuser() + "\n" | ||
| header = myheader + header | ||
|
|
||
| res = self.get_results_string(header, footer, update) | ||
| res = self.format_results_string(header, footer, update) | ||
| f = open(filename, "w") | ||
| f.write(res) | ||
| f.close() | ||
|
|
@@ -619,6 +634,67 @@ def saveResults(self, filename, header="", footer="", update=False): | |
| self.save_results(filename, header, footer, update) | ||
| return | ||
|
|
||
| @staticmethod | ||
| def _parse_results_text( | ||
| text: str, | ||
| ) -> dict[str, float | tuple[float, float]]: | ||
| known_metrics = { | ||
| "Residual", | ||
| "Contributions", | ||
| "Restraints", | ||
| "Chi2", | ||
| "Reduced", | ||
| "Rw", | ||
| } | ||
|
||
| parsed_results_dict = {} | ||
| for raw_line in text.splitlines(): | ||
| line = raw_line.strip() | ||
| if not line or set(line) == {"-"}: | ||
| continue | ||
| line_items = line.split() | ||
| if len(line_items) < 2: | ||
| continue | ||
| if line_items[0] == "Reduced" and len(line_items) >= 3: | ||
| try: | ||
| parsed_results_dict["Reduced Chi2"] = float(line_items[2]) | ||
|
||
| except ValueError: | ||
| pass | ||
| continue | ||
| if line_items[0] in known_metrics: | ||
| try: | ||
| parsed_results_dict[line_items[0]] = float(line_items[1]) | ||
| except ValueError: | ||
| pass | ||
| continue | ||
| if len(line_items) >= 4 and line_items[2] == "+/-": | ||
| try: | ||
| parsed_results_dict[line_items[0]] = ( | ||
| float(line_items[1]), | ||
| float(line_items[3]), | ||
| ) | ||
| except ValueError: | ||
| pass | ||
| return parsed_results_dict | ||
|
|
||
| def get_results_dictionary(self) -> dict[str, float | tuple[float, float]]: | ||
| """Parse all numeric results from the results text. | ||
|
|
||
| The method scans the entire results output and extracts all numeric | ||
| quantities. Two line formats are supported: | ||
|
|
||
| * ``name value`` → stored as a float | ||
| * ``name value +/- uncertainty`` → stored as ``(value, uncertainty)`` | ||
|
|
||
| Returns | ||
| ------- | ||
| results_dict : dict[str, float | tuple[float, float]] | ||
| Dictionary mapping result names to numeric values. Entries with | ||
| uncertainties are stored as ``(value, uncertainty)`` tuples. | ||
| """ | ||
| text = self.format_results_string() | ||
| results_dict = self._parse_results_text(text) | ||
| return results_dict | ||
|
|
||
|
|
||
| # End class FitResults | ||
|
|
||
|
|
@@ -750,8 +826,14 @@ def _calculate_metrics(self): | |
| # End class ContributionResults | ||
|
|
||
|
|
||
| @deprecated(resultsDictionary_dep_msg) | ||
| def resultsDictionary(results): | ||
| """Get dictionary of results from file. | ||
| """This function has been deprecated and will be removed in version 4.0.0. | ||
|
|
||
| Please use diffpy.srfit.fitbase.FitResults.get_results_dictionary instead. | ||
| See ``Examples of new usage`` below for usage. | ||
|
|
||
| Get dictionary of results from file. | ||
|
|
||
| This reads the results from file and stores them in a dictionary to be | ||
| returned to the caller. The dictionary may contain non-result entries. | ||
|
|
@@ -761,6 +843,16 @@ def resultsDictionary(results): | |
| results | ||
| An open file-like object, name of a file that contains | ||
| results from FitResults or a string containing fit results. | ||
|
|
||
| Examples of new usage | ||
| --------------------- | ||
| :: | ||
|
|
||
| from diffpy.srfit.fitbase import FitResults, FitRecipe | ||
|
|
||
| recipe = FitRecipe("my_recipe") | ||
| results = FitResults(recipe) | ||
| results_dict = results.get_results_dictionary() | ||
| """ | ||
| resstr = inputToString(results) | ||
|
|
||
|
|
@@ -777,8 +869,16 @@ def resultsDictionary(results): | |
| return mpairs | ||
|
|
||
|
|
||
| @deprecated(initializeRecipe_dep_msg) | ||
| def initializeRecipe(recipe, results): | ||
| """Initialize the variables of a recipe from a results file. | ||
| """This function has been deprecated and will be removed in version 4.0.0. | ||
|
|
||
| Please use | ||
| diffpy.srfit.fitbase.FitRecipe.initialize_recipe_from_results | ||
| instead. | ||
| For usage, see the ``Examples of new usage`` section below. | ||
|
|
||
| Initialize the variables of a recipe from a results file. | ||
|
|
||
| This reads the results from file and initializes any variables (fixed or | ||
| free) in the recipe to the results values. Note that the recipe has to be | ||
|
|
@@ -791,8 +891,60 @@ def initializeRecipe(recipe, results): | |
| results | ||
| An open file-like object, name of a file that contains | ||
| results from FitResults or a string containing fit results. | ||
| """ | ||
|
|
||
| Examples of new usage | ||
| ---------------------- | ||
| How to use the new initialization method in ``FitRecipe``. | ||
|
|
||
| Initialize a new FitRecipe from a results file | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
|
|
||
| :: | ||
|
|
||
| from diffpy.srfit.fitbase import FitRecipe, FitResults | ||
|
|
||
| new_recipe = FitRecipe("my_recipe") | ||
| new_recipe.create_new_variable("var1", 0) | ||
| new_recipe.create_new_variable("var2", 0) | ||
|
|
||
| # note: the variables in the new recipe (var1, var2) should match | ||
| # the variable names in the results file | ||
| path_to_results_file = "results.res" | ||
| new_recipe.initialize_recipe_from_results(path_to_results_file) | ||
|
|
||
| Initialize a FitRecipe from a FitResults object | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
|
|
||
| :: | ||
|
|
||
| from diffpy.srfit.fitbase import FitRecipe, FitResults | ||
| from scipy.optimize import leastsq | ||
|
|
||
| def optimize_recipe(recipe): | ||
| residuals = recipe.residual | ||
| values = recipe.values | ||
| leastsq(residuals, values) | ||
|
|
||
|
|
||
| profile = Profile() | ||
| x = linspace(0, pi, 10) | ||
| y = sin(x) | ||
| profile.set_observed_profile(x, y) | ||
| contribution = FitContribution("c1") | ||
| contribution.set_profile(profile) | ||
| contribution.set_equation("amplitude*sin(wave_number*x + phase_shift)") | ||
| recipe = FitRecipe() | ||
| recipe.add_contribution(contribution) | ||
| recipe.add_variable(contribution.amplitude, 1) | ||
| recipe.add_variable(contribution.wave_number, 1) | ||
| recipe.add_variable(contribution.phase_shift, 1) | ||
| optimize_recipe(recipe) | ||
|
|
||
| results = FitResults(recipe) | ||
|
|
||
| new_recipe = FitRecipe("my_recipe") | ||
| new_recipe.initialize_recipe_from_results(results) | ||
| """ | ||
| mpairs = resultsDictionary(results) | ||
| if not mpairs: | ||
| raise AttributeError("Cannot find results") | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.