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

[Merged by Bors] - feat: add norm_num extensions for factorials #8832

Closed
wants to merge 6 commits into from

Conversation

sebzim4500
Copy link
Collaborator

@sebzim4500 sebzim4500 commented Dec 5, 2023

Add norm_num extensions to evaluate Nat.factorial, Nat.ascFactorial and Nat.descFactorial.


Open in Gitpod

@sebzim4500 sebzim4500 added awaiting-review t-meta Tactics, attributes or user commands labels Dec 5, 2023
@sebzim4500 sebzim4500 requested a review from digama0 December 5, 2023 22:32
@robertylewis robertylewis added awaiting-author A reviewer has asked the author a question or requested changes and removed awaiting-review labels Dec 17, 2023
@alexjbest
Copy link
Member

@sebzim4500 do you want to push this over the finish line, or do you mind if someone else makes the changes to your PR for you? It would be great to have this in mathlib and the comments here are really just trivialities so I would be sad to see this stall forever!

@grunweg grunweg changed the title Add norm num extensions for factorials feat: add norm_num extensions for factorials Aug 21, 2024
@leanprover-community-bot-assistant leanprover-community-bot-assistant added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Nov 4, 2024
@sebzim4500 sebzim4500 force-pushed the sebzim4500/normnum-factorial branch from f0d58f1 to b24615f Compare March 4, 2025 21:28
@github-actions github-actions bot added new-contributor This PR was made by a contributor with at most 5 merged PRs. Welcome to the community! and removed merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) labels Mar 4, 2025
Copy link

github-actions bot commented Mar 4, 2025

PR summary ec497a5d28

Import changes for modified files

No significant changes to the import graph

Import changes for all files
Files Import difference
Mathlib.Tactic 1
Mathlib.Tactic.NormNum.NatFactorial (new file) 579

Declarations diff

+ ascFactorial_mul_ascFactorial
+ asc_factorial_aux
+ evalNatAscFactorial
+ evalNatDescFactorial
+ evalNatFactorial
+ isNat_ascFactorial
+ isNat_descFactorial
+ isNat_descFactorial_zero
+ isNat_factorial

You can run this locally as follows
## summary with just the declaration names:
./scripts/declarations_diff.sh <optional_commit>

## more verbose report:
./scripts/declarations_diff.sh long <optional_commit>

The doc-module for script/declarations_diff.sh contains some details about this script.


No changes to technical debt.

You can run this locally as

./scripts/technical-debt-metrics.sh pr_summary
  • The relative value is the weighted sum of the differences with weight given by the inverse of the current value of the statistic.
  • The absolute value is the relative value divided by the total sum of the inverses of the current values (i.e. the weighted average of the differences).

@sebzim4500 sebzim4500 removed the awaiting-author A reviewer has asked the author a question or requested changes label Mar 5, 2025
@sebzim4500 sebzim4500 force-pushed the sebzim4500/normnum-factorial branch from 7d572cc to ec497a5 Compare March 5, 2025 14:29
@sebzim4500
Copy link
Collaborator Author

Is there anything I can do to get this merged? I've resolved all the threads and CI is green.

@alexjbest
Copy link
Member

maintainer merge

Copy link

🚀 Pull request has been placed on the maintainer queue by alexjbest.

@kmill
Copy link
Contributor

kmill commented Mar 15, 2025

Thanks! I'm not really sure that this divide and conquer strategy would improve performance for the core algorithm itself (it results in large numbers being multiplied together, instead a large number multiplied by a small number, without saving any multiplications, and the latter tends to be faster). However: I think I buy the idea that this could improve the performance for kernel checking (or at least make larger factorials more accessible to the kernel), and I like the fact that it still allows the kernel to reduce 50 steps of ascFactorial on its own. In any case, it looks like it is correctly written, let's merge.

bors r+

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added ready-to-merge This PR has been sent to bors. and removed maintainer-merge labels Mar 15, 2025
mathlib-bors bot pushed a commit that referenced this pull request Mar 15, 2025
Add `norm_num` extensions to evaluate `Nat.factorial`, `Nat.ascFactorial` and `Nat.descFactorial`.



Co-authored-by: Eric Wieser <[email protected]>
@mathlib-bors
Copy link
Contributor

mathlib-bors bot commented Mar 15, 2025

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title feat: add norm_num extensions for factorials [Merged by Bors] - feat: add norm_num extensions for factorials Mar 15, 2025
@mathlib-bors mathlib-bors bot closed this Mar 15, 2025
@mathlib-bors mathlib-bors bot deleted the sebzim4500/normnum-factorial branch March 15, 2025 20:19
@sebzim4500
Copy link
Collaborator Author

Thanks! I'm not really sure that this divide and conquer strategy would improve performance for the core algorithm itself (it results in large numbers being multiplied together, instead a large number multiplied by a small number, without saving any multiplications, and the latter tends to be faster).

I tested it locally and the divide an conquer strategy was significantly faster at calculating e.g. 500!. It is very important to stop the divide and conquer approach once your blocks become sufficiently small though. It was not very sensitive to the actual threshold and a min block size of 20 to 100 all worked roughly as well as each other IIRC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-contributor This PR was made by a contributor with at most 5 merged PRs. Welcome to the community! ready-to-merge This PR has been sent to bors. t-meta Tactics, attributes or user commands
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants