Skip to content

Conversation

@proux01
Copy link
Collaborator

@proux01 proux01 commented Nov 24, 2024

Motivation for this change

Complete the development of itv.v and use it in place of signed.v

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

Reference: How to document

Reminder to reviewers

Fixes #879

Fixes #878

@proux01 proux01 force-pushed the deprecate_signed branch 4 times, most recently from 0300603 to 89faa96 Compare November 25, 2024 09:16
@affeldt-aist affeldt-aist self-requested a review December 3, 2024 06:58
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.

I haven't read in details but since it fixes at least two issues and since once merged the file will again be the object of a port to MathComp, I think it should be merged for the next release.

@proux01
Copy link
Collaborator Author

proux01 commented Feb 14, 2025

As you want. I still think it could be good to have a light review about for instance putting things in a Private module, checking that the doc is reasonnable,...

@proux01
Copy link
Collaborator Author

proux01 commented Feb 14, 2025

@affeldt-aist thanks for the fix/update but apparently there are still conflicts?

@affeldt-aist
Copy link
Member

As you want. I still think it could be good to have a light review about for instance putting things in a Private module, checking that the doc is reasonnable,...

You're right. I'll do that this week-end.

@affeldt-aist
Copy link
Member

@affeldt-aist thanks for the fix/update but apparently there are still conflicts?

Indeed. It compiles fine on my machine though (MathComp 2.3.0, HB 1.8.1, Coq 8.20.1).
I might try other configurations as part of my review this week-end.

@affeldt-aist
Copy link
Member

There is a number of definitions/lemmas (e.g., map_itv_bound, add_itv, see (non-exhaustive?) list below [1]) that do not depend on the Itv module but only on interval and possibly int. They amount for a sizeable amount of the file (about 20%?) and can be understood independently of the inference infrastructure. What about putting then together at the top of the file as a "preliminaries" section (extending interval.v) so as to lighten the core part of the file and clarify dependencies on section variables?

[1]
map_itv_bound
map_itv_bound_comp
opp_itv_bound
opp_itv_ge0
opp_itv_gt0
opp_itv
add_itv_boundl
add_itv_boundr
add_itv
mul_itv_boundl
mul_itv_boundr
signb
itv_bound_signl
itv_bound_signr
signi
interval_sign
mult_itv
mul_itv_boundrC
mul_itv_boundr_BRight
max_itv
min_itv
keep_pos_itv_bound
exprn_le1_itv_bound
exprn_itv

@proux01
Copy link
Collaborator Author

proux01 commented Feb 15, 2025

There is a number of definitions/lemmas (e.g., map_itv_bound, add_itv, see (non-exhaustive?) list below [1]) that do not depend on the Itv module but only on interval and possibly int. They amount for a sizeable amount of the file (about 20%?) and can be understood independently of the inference infrastructure. What about putting then together at the top of the file as a "preliminaries" section (extending interval.v) so as to lighten the core part of the file and clarify dependencies on section variables?

[1] map_itv_bound map_itv_bound_comp opp_itv_bound opp_itv_ge0 opp_itv_gt0 opp_itv add_itv_boundl add_itv_boundr add_itv mul_itv_boundl mul_itv_boundr signb itv_bound_signl itv_bound_signr signi interval_sign mult_itv mul_itv_boundrC mul_itv_boundr_BRight max_itv min_itv keep_pos_itv_bound exprn_le1_itv_bound exprn_itv

Note that most (if not all) of these are now in the Instances module which is not imported (and not meant to be). So it's a kind of private module. So we need to find a solution to not make them too public. Maybe a new IntervalArithmetic module, or even file?

@affeldt-aist
Copy link
Member

Should we merge this one for today's release?

And deprecate signed.

Co-authored-by: Reynald Affeldt <[email protected]>
Co-authored-by: Cyril Cohen <[email protected]>
@proux01
Copy link
Collaborator Author

proux01 commented Feb 19, 2025

Can be merged when CI is green

@affeldt-aist affeldt-aist merged commit 9863f51 into math-comp:master Feb 19, 2025
45 checks passed
@proux01 proux01 deleted the deprecate_signed branch February 20, 2025 16:30
proux01 added a commit to proux01/math-comp that referenced this pull request Feb 26, 2025
proux01 added a commit to proux01/math-comp that referenced this pull request Feb 26, 2025
proux01 added a commit to proux01/math-comp that referenced this pull request Feb 26, 2025
proux01 added a commit to proux01/math-comp that referenced this pull request Feb 27, 2025
proux01 added a commit to proux01/math-comp that referenced this pull request Feb 27, 2025
proux01 added a commit to proux01/math-comp that referenced this pull request Feb 27, 2025
proux01 added a commit to proux01/math-comp that referenced this pull request Feb 28, 2025
proux01 added a commit to proux01/math-comp that referenced this pull request Mar 5, 2025
proux01 added a commit to proux01/math-comp that referenced this pull request Mar 5, 2025
proux01 added a commit to CohenCyril/math-comp that referenced this pull request Mar 6, 2025
proux01 added a commit to proux01/math-comp that referenced this pull request Mar 6, 2025
proux01 added a commit to proux01/math-comp that referenced this pull request Mar 6, 2025
proux01 added a commit to proux01/math-comp that referenced this pull request Mar 7, 2025
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.

proof in itv.v could be improved with wlog numDomainType rather than realDomainType in itv.v

3 participants