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

ENH: Option to return covariance matrix of fitted coeffecients for polynomial fitters #7880

Closed
wants to merge 1 commit into from

Conversation

tom-bird
Copy link
Contributor

This is for fitters in the polynomial module.

Implementation is the same as numpy.polyfit.

See issue #7780. Was sent round the mailing list last week - no complaints.

…ents for the fitters in the polynomial module.

Implementation is the same as numpy.polyfit. See issue numpy#7780.
@maxnoe
Copy link

maxnoe commented Jul 10, 2019

This would be really helpful for us moving from numpy.polyfit to the options recommended by the docs.

What's the status here?

@@ -805,6 +817,9 @@ class domain in NumPy 1.4 and ``None`` in later versions.
if full:
[coef, status] = res
return cls(coef, domain=domain, window=window), status
elif cov:
Copy link

Choose a reason for hiding this comment

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

why is cov and full mutually exclusive? Shouldn't it be something like if full and not cov , elif cov and not full ....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose it is a little ambiguous. I was just copying the style of polyfit to keep it consistent: https://github.com/numpy/numpy/blob/d9b1e32cb8ef90d6b4a47853241db2a28146a57d/numpy/lib/polynomial.py#L639-L6419

Base automatically changed from master to main March 4, 2021 02:03
@seberg seberg requested a review from charris September 8, 2021 16:50
eliottrosenberg added a commit to eliottrosenberg/numpy that referenced this pull request Sep 16, 2022
Implementing changes from
numpy#7880
eliottrosenberg added a commit to eliottrosenberg/numpy that referenced this pull request Sep 16, 2022
eliottrosenberg added a commit to eliottrosenberg/numpy that referenced this pull request Sep 16, 2022
eliottrosenberg added a commit to eliottrosenberg/numpy that referenced this pull request Sep 16, 2022
eliottrosenberg added a commit to eliottrosenberg/numpy that referenced this pull request Sep 16, 2022
eliottrosenberg added a commit to eliottrosenberg/numpy that referenced this pull request Sep 16, 2022
eliottrosenberg added a commit to eliottrosenberg/numpy that referenced this pull request Sep 16, 2022
eliottrosenberg added a commit to eliottrosenberg/numpy that referenced this pull request Sep 16, 2022
@InessaPawson InessaPawson added the triage review Issue/PR to be discussed at the next triage meeting label Sep 16, 2022
@mattip
Copy link
Member

mattip commented Oct 5, 2022

We discussed this at a recent triage meeting and the general opinion was to add a function fit_with_covariance instead of adding another kwarg. It would be nice to move this forward for feature parity.

@mattip mattip removed the triage review Issue/PR to be discussed at the next triage meeting label Oct 5, 2022
@charris
Copy link
Member

charris commented Oct 31, 2023

The changes should be mostly be in polyutils._fit. Note that it is private, so you can add arguments.

la.inv(np.dot(lhs.T, lhs)) is not a great way to compute the covariance matrix, but probably OK for well posed problems.

Docstrings need .. versionadded:: lines for the cov argument.

Maybe we should add a slot for the covariance in the polynomial base class? That would allow transformation of domain and polynomial basis types to track it. The default value would be None.

Note that number of degrees of freedom used to estimate the measurement variance can become uncertain, depending on how the weights are used and whether some variables are lost due to the condition number.

@mattip
Copy link
Member

mattip commented Jul 3, 2024

I am going to close this since it has been inactive for nearly two years. It still would be nice to get a fit_with_covariance function. Please feel free to reopen and continue or open a new PR to move this forward.

@mattip mattip closed this Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Pending author's response
Development

Successfully merging this pull request may close these issues.

None yet

5 participants