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

Remove duplicated lemmas and dependency to Rstruct #1347

Merged
merged 1 commit into from
Oct 17, 2024

Conversation

proux01
Copy link
Collaborator

@proux01 proux01 commented Oct 11, 2024

Motivation for this change

This both removes copy/pasted lemmas (the removed lemmas are still in Rstruct.v) and an undue dependency to Rstruct.

Checklist
  • [ ] added corresponding entries in CHANGELOG_UNRELEASED.md
  • [ ] added corresponding documentation in the headers

Reference: How to document

Reminder to reviewers

@proux01 proux01 mentioned this pull request Oct 11, 2024
4 tasks
Copy link
Contributor

@zstone1 zstone1 left a comment

Choose a reason for hiding this comment

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

Very happy to see code deletions. Just a question about if there is a way to instantiate a realType without RStruct.

@@ -3708,7 +3640,7 @@ exists (Uniform.class T'), ([set xy | ball (f xy.1) 1 (f xy.2)]); split.
Qed.

Section normalP.
Context {T : topologicalType}.
Context {R : realType} {T : topologicalType}.

Let normal_spaceP : [<->
Copy link
Contributor

Choose a reason for hiding this comment

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

So the reason I didn't do this before is that none of the clauses in normal_spaceP actually depend on R. It's only used internally in the proofs. So if there's a preferred way to show that there is an instance of realType, that would be great. If not then I am ok with this change if removing the dependency is helpful.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the explanation, I see what you intended there now. But while this makes perfect sense from a mathematical point of view, it seems much more dubiuous from a software engineering point of view. This dependency to Rstruct pull a complete dependency to the Stdlib, that is 200k+ lines of code (something about the size of mathcomp) and a bunch of specific axioms (that you can see with Print Assumptions).
Indeed, the goal of Rstruct.v is not to provide an instance of realType (although it incidentally does) but rather to provide a bridge between Analysis and the reals from Stdlib.
I guess providing an instance of realType was the goal of the developments in altreals but this never got completed. @affeldt-aist @CohenCyril what's your opinion?

Copy link
Member

Choose a reason for hiding this comment

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

I guess providing an instance of realType was the goal of the developments in altreals but this never got completed. @affeldt-aist @CohenCyril what's your opinion?

That's a tough one. I think that @drouhling and @CohenCyril worked hard to remove the dependency to R in topology.v, that's a different problem, sure, but in the same vein I think that the end-user might not want to see such a dependency if does not appear explicitly in the clauses of the statement.

Would the availability of a model relying only on MathComp-Analysis be a good compromise? Because @strub started the following one (@proux01 : by any chance, are referring to this when you talk about the goal of altreals?)

#116

iirc the work was on the verge of completion. It is maybe time to reboot it, we have been postponing it from one release to the other for a long time...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe, my understanding of altreals is very poor.
Indeed, it would be good to complete a proper instance of realType. That may also be a good thing for users avoinding them to always need that strange Variable R : realType everywhere in the code.
Meanwhile, maybe a good compromise for this PR is to keep a comment mentioning that we should use that instance as soon as we get it?

Copy link
Contributor

Choose a reason for hiding this comment

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

A MCA only instance of the reals would be the nicest solution here. But in the meanwhile the extra R parameter seems a small price to pay for the removing that dependency.

Copy link
Member

Choose a reason for hiding this comment

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

We should maybe keep an issue with a permalink to the incriminated proof after merge to keep track of this need.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Opened #1355 to remember (and added a comment in the code)

@proux01
Copy link
Collaborator Author

proux01 commented Oct 15, 2024

So this should now be ready

Copy link
Member

@affeldt-aist affeldt-aist left a comment

Choose a reason for hiding this comment

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

and that's great to have normedtype.v started to be cleaned up!

@proux01
Copy link
Collaborator Author

proux01 commented Oct 17, 2024

So @affeldt-aist should this be merged?

@affeldt-aist affeldt-aist merged commit 6dfceaa into math-comp:master Oct 17, 2024
16 checks passed
@proux01 proux01 deleted the rm_duplication branch October 17, 2024 12:20
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.

3 participants