-
Notifications
You must be signed in to change notification settings - Fork 49
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
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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 onR
. It's only used internally in the proofs. So if there's a preferred way to show that there is an instance ofrealType
, that would be great. If not then I am ok with this change if removing the dependency is helpful.There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)