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

New Feature: Fix and improve coeftable for optimize() output #2034

Merged
merged 11 commits into from
Jul 12, 2023
Merged

New Feature: Fix and improve coeftable for optimize() output #2034

merged 11 commits into from
Jul 12, 2023

Conversation

DominiqueMakowski
Copy link
Contributor

@DominiqueMakowski DominiqueMakowski commented Jul 10, 2023

This PR aims at the following:

  • Fixes coeftable on optimize output (MLE and MAP) throws error #2033 by converting the symbols to String
  • Following this comment, it would be super nice to add indices typically present in coeftables, such as p-values and confidence intervals. I have added (commented out) the code to do so, and I'm happy to include it.
  • I would also suggest to rename the columns to make it more comparable to a regular coeftable
coeftable(GLM.lm(@formula(y ~ x), DataFrame(x=x, y=y)))
────────────────────────────────────────────────────────────────────────
                 Coef.  Std. Error     t  Pr(>|t|)  Lower 95%  Upper 95%
────────────────────────────────────────────────────────────────────────
(Intercept)  22.2225      3.59373   6.18    <1e-06  14.9474    29.4977
x             0.482927    0.152752  3.16    0.0031   0.173697   0.792157
────────────────────────────────────────────────────────────────────────
  • However, I would make one exception: rename t as z because as far as I understood this is a z approximation, not a t-based estimation. I know that in some cases, the parameter is named t even though it's a z, but I think we could avoid confusion by being transparent

Note: the only changes were done for StatsBase.coeftable, the rest is formatting fixes (removal trailing spaces) done by the autoformatter.

Let me know if I should add the suggested changes :)

@yebai
Copy link
Member

yebai commented Jul 10, 2023

Thanks, @DominiqueMakowski -- it looks like very sensible improvements!

@DominiqueMakowski
Copy link
Contributor Author

Done! I've exposed the level parameters to match StatsBase

Also, but this is more a question, would it be in principle feasible to add a predict() method to predict data based on the MLE estimates?

@yebai
Copy link
Member

yebai commented Jul 10, 2023

Also, but this is more a question, would it be in principle feasible to add a predict() method to predict data based on the MLE estimates?

There have been discussions about adding predict to Turing, although that PR has not been merged yet. Feel free to open another PR for predict to Turing.jl in the meantime.

cc @sethaxen

@torfjelde
Copy link
Member

FYI we already have a predict @yebai , it just doesn't do what @DominiqueMakowski mentioned:) But yeah, such a method would be nice to add!

@yebai yebai requested a review from cpfiffer July 10, 2023 16:15
@DominiqueMakowski
Copy link
Contributor Author

Feel free to open another PR for predict to Turing.jl in the meantime.

That's probably beyond my skills, I'm not even sure where I'd need to start

Copy link
Member

@torfjelde torfjelde left a comment

Choose a reason for hiding this comment

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

This looks good to me:) Good stuff @DominiqueMakowski !

I'm letting someone else hit the "approve" button though, as I was not particularly invovled in the original impl of this mode-estimation stuff, and so don't know if there are any particular concerns regarding changing the outputs (though from a quick glance, I agree with what you're saying, so it all seems sensible to me).

@cpfiffer you have any thoughts?

@sethaxen
Copy link
Member

There have been discussions about adding predict to Turing, although that PR has not been merged yet. Feel free to open another PR for predict to Turing.jl in the meantime.

It may have been a mistake to try to get the predict overload into AbstractPPL. The original issue was TuringLang/DynamicPPL.jl#466.

@torfjelde
Copy link
Member

Btw @DominiqueMakowski can you bump the patch version?

src/modes/OptimInterface.jl Outdated Show resolved Hide resolved
src/modes/OptimInterface.jl Outdated Show resolved Hide resolved
src/modes/OptimInterface.jl Outdated Show resolved Hide resolved
DominiqueMakowski and others added 3 commits July 10, 2023 18:22
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
@DominiqueMakowski
Copy link
Contributor Author

Thanks @devmotion !

Btw @DominiqueMakowski can you bump the patch version?

How do I do that?

@coveralls
Copy link

coveralls commented Jul 10, 2023

Pull Request Test Coverage Report for Build 5510378272

  • 0 of 11 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 0.0%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/modes/OptimInterface.jl 0 11 0.0%
Totals Coverage Status
Change from base Build 5510129926: 0.0%
Covered Lines: 0
Relevant Lines: 1448

💛 - Coveralls

@codecov
Copy link

codecov bot commented Jul 10, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (90e1d21) 0.00% compared to head (b27b394) 0.00%.

Additional details and impacted files
@@          Coverage Diff           @@
##           master   #2034   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files          22      22           
  Lines        1444    1448    +4     
======================================
- Misses       1444    1448    +4     
Impacted Files Coverage Δ
src/modes/OptimInterface.jl 0.00% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

src/modes/OptimInterface.jl Outdated Show resolved Hide resolved
Copy link
Member

@cpfiffer cpfiffer left a comment

Choose a reason for hiding this comment

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

This is fantastic thank you for doing this! I left one minor comment but for the most part I'd be happy to merge this after the tests pass.

src/modes/OptimInterface.jl Outdated Show resolved Hide resolved
@DominiqueMakowski
Copy link
Contributor Author

but for the most part I'd be happy to merge this after the tests pass.

We bumped the version, I'm not sure the failing tests are related to this PR

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
@cpfiffer cpfiffer merged commit 59e5cce into TuringLang:master Jul 12, 2023
@cpfiffer
Copy link
Member

I'm on my phone, could someone tag the registrator?

@yebai
Copy link
Member

yebai commented Jul 12, 2023

@cpfiffer Just tagged a release.

@storopoli storopoli changed the title New Feature: Fix and improve coeftable for otpimize() output New Feature: Fix and improve coeftable for optimize() output Jul 12, 2023
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.

coeftable on optimize output (MLE and MAP) throws error
7 participants