Skip to content
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

Introduce heuristic attribute and change parameter initialisation #45

Merged
merged 6 commits into from
Nov 9, 2022

Conversation

AUTProgram
Copy link
Contributor

@AUTProgram AUTProgram commented Nov 8, 2022

This PR modifies the way in which initial values for parameters are obtained. An attribute heuristic has been added to the ModelParameter class, which is used if both of fixed_to and initialised_to are None. There is now only one method to get the initial guess for parameters during fitting, called get_initial_value. This method may return None only before estimate_parameters has been called. If get_initial_value returns None for any parameter after pre-fit estimation has been completed, the fitter raises an error.

As part of this PR, bound checking for fixed parameters is added. Functionality is further changed such that if the initial value is outside bounds, an error is thrown rather than silently clipping the parameter.

@AUTProgram
Copy link
Contributor Author

I have run unit tests, all work except one that throws

message = 'Matplotlib is currently using agg, which is a non-GUI backend, so cannot show the figure.', category = None

    def warn_external(message, category=None):
        """
        `warnings.warn` wrapper that sets *stacklevel* to "outside Matplotlib".

        The original emitter of the warning can be obtained by patching this
        function back to `warnings.warn`, i.e. ``_api.warn_external =
        warnings.warn`` (or ``functools.partial(warnings.warn, stacklevel=2)``,
        etc.).
        """
        frame = sys._getframe()
        for stacklevel in itertools.count(1):  # lgtm[py/unused-loop-variable]
            if frame is None:
                # when called in embedded context may hit frame is None
                break
            if not re.match(r"\A(matplotlib|mpl_toolkits)(\Z|\.(?!tests\.))",
                            # Work around sphinx-gallery not setting __name__.
                            frame.f_globals.get("__name__", "")):
                break
            frame = frame.f_back
>       warnings.warn(message, category, stacklevel)
E       UserWarning: Matplotlib is currently using agg, which is a non-GUI backend, so cannot show the figure.

..\..\AppData\Local\pypoetry\Cache\virtualenvs\ionics-fits-fyXcmhes-py3.8\lib\site-packages\matplotlib\_api\__init__.py:363: UserWarning
================================================================================================================================================= short test summary info ==================================================================================================================================================
FAILED test/test_polynomial.py::test_power_n - UserWarning: Matplotlib is currently using agg, which is a non-GUI backend, so cannot show the figure.

Not sure if this is caused by the configuration of the pc I am using.

ionics_fits/common.py Outdated Show resolved Hide resolved
Copy link
Contributor

@hartytp hartytp left a comment

Choose a reason for hiding this comment

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

Overall, I think this is a nice positive change.

A few stylistic points to consider. The most substantive thing is the unwanted change of behaviour by no longer using x = self.parameters["x"].initialise(value).

ionics_fits/common.py Outdated Show resolved Hide resolved
ionics_fits/common.py Show resolved Hide resolved
ionics_fits/common.py Outdated Show resolved Hide resolved
ionics_fits/common.py Outdated Show resolved Hide resolved
ionics_fits/common.py Outdated Show resolved Hide resolved
ionics_fits/common.py Show resolved Hide resolved
ionics_fits/common.py Outdated Show resolved Hide resolved
ionics_fits/common.py Outdated Show resolved Hide resolved
ionics_fits/common.py Show resolved Hide resolved
ionics_fits/models/lorentzian.py Outdated Show resolved Hide resolved
@hartytp hartytp mentioned this pull request Nov 8, 2022
@AUTProgram
Copy link
Contributor Author

AUTProgram commented Nov 9, 2022

All tests finish correctly

@AUTProgram
Copy link
Contributor Author

@hartytp Let me know if you are happy with the docstrings now.

@hartytp
Copy link
Contributor

hartytp commented Nov 9, 2022

@hartytp Let me know if you are happy with the docstrings now.

which one?

ionics_fits/common.py Outdated Show resolved Hide resolved
@AUTProgram
Copy link
Contributor Author

which one?

Those for get_initial_value and estimate_parameters mainly. I will have to copy the docstring of estimate_parameters for each model.

ionics_fits/common.py Outdated Show resolved Hide resolved
@AUTProgram
Copy link
Contributor Author

I believe this is ready to be merged

@hartytp
Copy link
Contributor

hartytp commented Nov 9, 2022

@AUTProgram AUTProgram merged commit a492d38 into master Nov 9, 2022
@AUTProgram AUTProgram deleted the initial-value-update branch November 9, 2022 16:36
@hartytp
Copy link
Contributor

hartytp commented Nov 9, 2022

🎆

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.

2 participants