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(Probability/Kernel/CondCdf): cleanup #10635

Closed
wants to merge 17 commits into from

Conversation

mo271
Copy link
Collaborator

@mo271 mo271 commented Feb 16, 2024

@mo271 mo271 marked this pull request as draft February 16, 2024 13:03
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the blocked-by-other-PR This PR depends on another PR which is still in the queue. label Feb 16, 2024
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot removed the blocked-by-other-PR This PR depends on another PR which is still in the queue. label Mar 26, 2024
@leanprover-community-mathlib4-bot
Copy link
Collaborator

@mo271 mo271 marked this pull request as ready for review March 26, 2024 18:18
@mo271 mo271 added the awaiting-review The author would like community review of the PR label Mar 26, 2024
Copy link
Contributor

@RemyDegenne RemyDegenne left a comment

Choose a reason for hiding this comment

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

Can you comment on the change of imports? Does that add many files?

Mathlib/Data/Real/Archimedean.lean Outdated Show resolved Hide resolved
Mathlib/Data/Real/Archimedean.lean Outdated Show resolved Hide resolved
Mathlib/Data/Real/Archimedean.lean Outdated Show resolved Hide resolved
@RemyDegenne RemyDegenne 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 Mar 27, 2024
mo271 and others added 3 commits March 27, 2024 08:53
Co-authored-by: Rémy Degenne <remydegenne@gmail.com>
Co-authored-by: Rémy Degenne <remydegenne@gmail.com>
Co-authored-by: Rémy Degenne <remydegenne@gmail.com>
Mathlib/Data/Real/Archimedean.lean Outdated Show resolved Hide resolved
Mathlib/Data/Real/Archimedean.lean Outdated Show resolved Hide resolved
@mo271
Copy link
Collaborator Author

mo271 commented Mar 27, 2024

Can you comment on the change of imports? Does that add many files?

I replaced import Mathlib.Data.Set.Image with Mathlib.Data.Set.Intervals.Disjoint and sorted the import alphabetically. Not sure how many more files that makes. Is there an easy way to list all imports of a file recursively?

@RemyDegenne
Copy link
Contributor

I asked around how to check imports: lake exe graph --to X --from Y out.pdf can give a view of the imports between files, one you have cache. In this case, it looks like you added almost nothing.

@mo271
Copy link
Collaborator Author

mo271 commented Mar 27, 2024

All the imports that are additionally imported recursively due to this change:

https://gist.github.com/mo271/3c31f22eee38698d7bd9113f4f1b4e64

This is much longer than the output of the command you suggested:

lake exe graph --reduce --from Mathlib.Data.Set.Image --to Mathlib.Data.Set.Intervals.Disjoint  diff.svg

which is just this image:
diff

@RemyDegenne
Copy link
Contributor

The command I listed gives only the files between the two files in the command. That's indeed not the same as the files additionally imported.
I don't understand the list you posted. Can you explain the format "Mathlib.Tactic.GuardHypNums" -> "Mathlib.Tactic.Common"; ?

@mo271
Copy link
Collaborator Author

mo271 commented Mar 27, 2024

Yeah let me explain, I arrived at the list in the gist with the following commands:

lake exe graph --reduce  --to Mathlib.Data.Set.Image all_image.dot
lake exe graph --reduce  --to Mathlib.Data.Set.Intervals.Disjoint  all_disjoint.dot
sort all_disjoint.dot > all_disjoint.dot.sorted
sort all_image.dot > all_image.dot.sorted
comm -23 all_disjoint.dot.sorted all_image.dot.sorted

So this basically list all additional edges in the import graph

@mo271
Copy link
Collaborator Author

mo271 commented Mar 27, 2024

by the way, I think I will propose to add functionality to the ImportGraph package to just get a list of imports as a sorted list instead of the entire graph...

@RemyDegenne
Copy link
Contributor

If I understand correctly, the interesting info in the list you posted is the list of files that appear in the list of edges. Those are the files imported by the new import and not the old one.

But what really interest us is the difference between the list of files imported by the union of all new imports, minus the list of files imported by the union of all old imports. It might be that Mathlib.Data.Set.Intervals.Disjoint imports many more files than Mathlib.Data.Set.Image but those files might be mostly imported by other import lines anyway.

@mo271
Copy link
Collaborator Author

mo271 commented Mar 27, 2024

That makes sense, calculating only the new edges by running

lake exe graph --reduce --to Mathlib.Data.Real.Archimedean all.dot

twice once with the new imports in place and once with only the old imports (building lake build Mathlib.Data.Real.Archimedean in between) gives only this diff

  "Mathlib.Data.Set.Intervals.Disjoint" -> "Mathlib.Data.Real.Archimedean";
  "Mathlib.Data.Set.Lattice" -> "Mathlib.Data.Set.Intervals.Disjoint";

So the only extra file seems to be Mathlib.Data.Set.Lattice

Copy link
Contributor

@RemyDegenne RemyDegenne left a comment

Choose a reason for hiding this comment

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

Great! Thanks for investigating the import issue! It would be very nice to get a bot to do that for us on PRs.
bors d+

Mathlib/Data/Real/Archimedean.lean Show resolved Hide resolved
@mathlib-bors
Copy link

mathlib-bors bot commented Mar 27, 2024

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

Co-authored-by: Rémy Degenne <remydegenne@gmail.com>
@mo271
Copy link
Collaborator Author

mo271 commented Mar 27, 2024

bors r+

mathlib-bors bot pushed a commit that referenced this pull request Mar 27, 2024
- [x] depends on: #10298 


Co-authored-by: Moritz Firsching <firsching@google.com>
@mathlib-bors
Copy link

mathlib-bors bot commented Mar 27, 2024

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title chore(Probability/Kernel/CondCdf): cleanup [Merged by Bors] - chore(Probability/Kernel/CondCdf): cleanup Mar 27, 2024
@mathlib-bors mathlib-bors bot closed this Mar 27, 2024
@mathlib-bors mathlib-bors bot deleted the mo271/notBddAbove_coe branch March 27, 2024 14:30
Louddy pushed a commit that referenced this pull request Apr 15, 2024
- [x] depends on: #10298 


Co-authored-by: Moritz Firsching <firsching@google.com>
atarnoam pushed a commit that referenced this pull request Apr 16, 2024
- [x] depends on: #10298 


Co-authored-by: Moritz Firsching <firsching@google.com>
uniwuni pushed a commit that referenced this pull request Apr 19, 2024
- [x] depends on: #10298 


Co-authored-by: Moritz Firsching <firsching@google.com>
callesonne pushed a commit that referenced this pull request Apr 22, 2024
- [x] depends on: #10298 


Co-authored-by: Moritz Firsching <firsching@google.com>
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 delegated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants