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] - chore: change the definition of List.finRange #19447

Closed
wants to merge 11 commits into from

Conversation

kim-em
Copy link
Contributor

@kim-em kim-em commented Nov 25, 2024

François Dorais has been working on upstreaming List.finRange, but wants to change the definition at the same time. That was running into difficulties, which hopefully are resolved here.

@github-actions github-actions bot added the large-import Automatically added label for PRs with a significant increase in transitive imports label Nov 25, 2024
Copy link

github-actions bot commented Nov 25, 2024

PR summary c25fea8a05

Import changes exceeding 2%

% File
+2.61% Mathlib.Data.List.Sublists

Import changes for modified files

Dependency changes

File Base Count Head Count Change
Mathlib.Data.List.Range 339 287 -52 (-15.34%)
Mathlib.Data.List.Sublists 345 354 +9 (+2.61%)
Mathlib.Data.List.FinRange 351 348 -3 (-0.85%)
Import changes for all files
Files Import difference
Mathlib.Data.List.Range -52
Mathlib.Data.List.FinRange -3
Mathlib.Data.Finset.Powerset 5
Mathlib.Algebra.BigOperators.Ring.Multiset 8
3 files Mathlib.Data.Multiset.Antidiagonal Mathlib.Data.List.Sublists Mathlib.Data.Multiset.Powerset
9

Declarations diff

+ finRange_eq_pmap_range

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.


Increase in tech debt: (relative, absolute) = (1.00, 0.01)
Current number Change Type
184 1 adaptation notes

Current commit c25fea8a05
Reference commit b96544419e

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).

@@ -488,9 +488,12 @@ theorem length_mapAccumr₂ :

end MapAccumr

/- #adaptation_note: this attribute should be removed after Mathlib moves to v4.15.0-rc1. -/
set_option allowUnsafeReducibility true in
attribute [semireducible] Fin.foldr.loop
Copy link
Member

Choose a reason for hiding this comment

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

Does this apply globally, or just for the current file?

Copy link
Member

Choose a reason for hiding this comment

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

I also worry that this PR might have accumulated a bunch of proof uglification before we discovered this fix; is that the case, or do I have the chronology wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The uglification seems to still be necessary at the moment, and I'm not really sure why it is happening. It is only at the level of having to redo some simp only blocks, however.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this apply globally, or just for the current file?

This is global.

@github-actions github-actions bot removed the large-import Automatically added label for PRs with a significant increase in transitive imports label Nov 25, 2024
@github-actions github-actions bot added the large-import Automatically added label for PRs with a significant increase in transitive imports label Nov 25, 2024
Copy link
Member

@jcommelin jcommelin left a comment

Choose a reason for hiding this comment

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

Thanks 🎉

If CI passes, please remove the label awaiting-CI and merge this yourself, by adding a comment bors r+.

bors d+

@mathlib-bors
Copy link
Contributor

mathlib-bors bot commented Nov 25, 2024

✌️ kim-em can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@kim-em
Copy link
Contributor Author

kim-em commented Nov 25, 2024

bors merge

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the ready-to-merge This PR has been sent to bors. label Nov 25, 2024
mathlib-bors bot pushed a commit that referenced this pull request Nov 25, 2024
François Dorais has been [working](leanprover-community/batteries#1055) on upstreaming `List.finRange`, but wants to change the definition at the same time. That was running into difficulties, which hopefully are resolved here.
@mathlib-bors
Copy link
Contributor

mathlib-bors bot commented Nov 25, 2024

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title chore: change the definition of List.finRange [Merged by Bors] - chore: change the definition of List.finRange Nov 25, 2024
@mathlib-bors mathlib-bors bot closed this Nov 25, 2024
@mathlib-bors mathlib-bors bot deleted the finRange_def branch November 25, 2024 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
delegated large-import Automatically added label for PRs with a significant increase in transitive imports ready-to-merge This PR has been sent to bors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants