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

fix: fin bug in to_additive #1861

Closed
wants to merge 5 commits into from
Closed

fix: fin bug in to_additive #1861

wants to merge 5 commits into from

Conversation

fpvandoorn
Copy link
Member

@fpvandoorn fpvandoorn commented Jan 26, 2023

  • The to_additive heuristic was ported wrong (a conjunction in Lean 3 was ported to a disjunction in Lean 4)
    • I had to add a check to take care that heuristic doesn't treat (fun x => α) (n + 1) as a type that depends on Nat. This was causing at least one error.
  • Also re-add some simple debug tracing in applyReplacementFun (with dbg_trace, since it's a pure function).
  • In a future PR I will speed-up additiveTestAux, since it's currently still exponentially slower than necessary (though usually applied to only small terms).

Open in Gitpod

@fpvandoorn fpvandoorn added awaiting-review The author would like community review of the PR t-meta Tactics, attributes or user commands labels Jan 26, 2023
@fpvandoorn fpvandoorn changed the title fix: fix fin bug in to_additive fix: fin bug in to_additive Jan 26, 2023
Lean.Expr.replaceRec fun r e ↦ Id.run do
if trace then
dbg_trace s!"replacing at {e}"
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's a good idea to use dbg_trace for logging. It doesn't use MessageData, and the output is not interleaved with the other trace messages (dbg_trace just prints things to stdout).

We should either put this function in CoreM or leave out the logging.

Copy link
Member Author

@fpvandoorn fpvandoorn Feb 3, 2023

Choose a reason for hiding this comment

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

Putting this function in CoreM would require generalizing memoFix to monads (which is used in Lean.Expr.replaceRec), so that it memoizes the output value of a monad application. I don't know if that is doable, and more specifically, I don't know how to do that.

I find the dbg_trace useful when debugging, and they will only ever fire with trace.to_additive_detail set to true, which almost nobody ever does (and never happens on the master branch of mathlib). That said, if you think this bad, I'll remove the dbg_trace (I could also put them behind a new option to_additive_debug).

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I didn't realize it was only set with to_additive_detail. I still don't think it's a good solution, but at least it's contained.

@gebner gebner added awaiting-author A reviewer has asked the author a question or requested changes and removed awaiting-review The author would like community review of the PR labels Feb 3, 2023
@semorrison semorrison added the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Feb 3, 2023
@fpvandoorn
Copy link
Member Author

I was very confused when merging this with master, since git would remove all my changes.
Apparently what happened is that this PR was already merged to master as a part of #1848. (cc @jcommelin @joneugster)

@fpvandoorn
Copy link
Member Author

fpvandoorn commented Feb 3, 2023

I will close this PR now

@gebner I will still change the dbg_trace to whatever you prefer, see above.

@fpvandoorn fpvandoorn closed this Feb 3, 2023
@joneugster
Copy link
Collaborator

I was very confused when merging this with master, since git would remove all my changes.
Apparently what happened is that this PR was already merged to master as a part of #1848. (cc @jcommelin @joneugster)

Looks like #1848 has been merged while still having a "Blocked-by-other-PR" label.

@fpvandoorn fpvandoorn deleted the toadditivefinbug branch March 1, 2023 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-author A reviewer has asked the author a question or requested changes merge-conflict The PR has a merge conflict with master, and needs manual merging. t-meta Tactics, attributes or user commands
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants