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

nat: least common multiple #2207

Merged
merged 8 commits into from
Mar 13, 2025

Conversation

Alizter
Copy link
Collaborator

@Alizter Alizter commented Jan 23, 2025

No description provided.

@Alizter Alizter requested a review from jdchristensen January 28, 2025 19:33
destruct l; only 1: exact _.
intros [k p].
exists k.
nrapply nat_div_moveL_nV.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be good to figure out why typeclass search spins if you use rapply here. We must have some bad choices of instances.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FTR:

Calling typeclass resolution with flags: depth = 5,unique = false,fail = false
1: looking for (0 < ?k) with backtracking
1.1: simple apply @lt_succ_r on (0 < ?k), 1 subgoal(s)
1.1-1 : (0 < ?m0)
1.1-1: looking for (0 < ?m0) with backtracking
1.1-1.1: simple apply @lt_succ_r on (0 < ?m0), 1 subgoal(s)
1.1-1.1-1 : (0 < ?m0)
1.1-1.1-1: looking for (0 < ?m0) with backtracking
1.1-1.1-1.1: simple apply @lt_succ_r on (0 < ?m0), 1 subgoal(s)
1.1-1.1-1.1-1 : (0 < ?m0)
1.1-1.1-1.1-1: looking for (0 < ?m0) with backtracking
1.1-1.1-1.1-1.1: simple apply @lt_succ_r on (0 < ?m0), 1 subgoal(s)
1.1-1.1-1.1-1.1-1 : (0 < ?m0)
1.1-1.1-1.1-1.1-1: looking for (0 < ?m0) with backtracking
1.1-1.1-1.1-1.1-1.1: simple apply @lt_succ_r on (0 < ?m0), 1 subgoal(s)
1.1-1.1-1.1-1.1-1.1-1 : (0 < ?m0)
1.1-1.1-1.1-1.1-1.2: unfold lt on (0 < ?m0), 1 subgoal(s)
1.1-1.1-1.1-1.1-1.2-1 : (1 <= ?m0)
1.1-1.1-1.1-1.1-1.3: simple apply lt_is_leq on (0 < ?m0), 1 subgoal(s)
1.1-1.1-1.1-1.1-1.3-1 : (1 <= ?m0)
1.1-1.1-1.1-1.2: unfold lt on (0 < ?m0), 1 subgoal(s)
1.1-1.1-1.1-1.2-1 : (1 <= ?m0)

lt_succ_r is used in a few places. I'll see about pruning it so it doesn't get used too often.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed by making lt_succ_r lower priority. It was being used somewhere for FinNat and caused an error, but this was fixed by explicitly giving the expected instance.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But won't it still potentially loop if other instances don't solve the goal? Would it be better to constrain it with Hint Mode so that it only applies when the goal is of the form ?n < constructor?

BTW, this part

1.1-1.1-1.1-1.1-1.2: unfold lt on (0 < ?m0), 1 subgoal(s)
1.1-1.1-1.1-1.1-1.2-1 : (1 <= ?m0)
1.1-1.1-1.1-1.1-1.3: simple apply lt_is_leq on (0 < ?m0), 1 subgoal(s)
1.1-1.1-1.1-1.1-1.3-1 : (1 <= ?m0)

is because of

#[export] Hint Unfold lt : typeclass_instances.
Global Instance lt_is_leq n m : leq n.+1 m -> lt n m | 100 := idmap.

Isn't the Hint Unfold line redundant given the second line? Or vice versa? (And same for the other two inequalities.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll remove the instances, the unfold hint is more general. For example, IsHProp (a < b) can be solved.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wasn't able to get Hint Mode to work here.

Defined.

(** [n] divides the least common multiple of [n] and [m]. *)
Global Instance nat_divides_r_lcm_l n m : (n | nat_lcm n m) := _.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see you are using divides_l and divides_r for the next few results, but maybe it's better to put the thing before or after the word "divides"? OTOH, if this is how it was done systematically in the rest of the file, I guess it's ok.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I always put it after given that's how we do it elsewhere. Naming things is hard, but I guess its better to be consistent. Iff doesn't follow this convention, but maybe it should for consistency.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here I guess that last _l indicates that the first argument of lcm is "special" in that it is the n that appears on the left of the divides relation? Not obvious, but reasonable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes that's the correct interpretation.

Copy link
Collaborator

@jdchristensen jdchristensen left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in getting back to this!

@Alizter
Copy link
Collaborator Author

Alizter commented Mar 13, 2025

I'll quickly rebase this.

Alizter and others added 8 commits March 13, 2025 15:44
Signed-off-by: Ali Caglayan <alizter@gmail.com>

<!-- ps-id: b14fcb1b-6f31-48e9-b535-c8b57935883d -->
Signed-off-by: Ali Caglayan <alizter@gmail.com>
Signed-off-by: Ali Caglayan <alizter@gmail.com>
Signed-off-by: Ali Caglayan <alizter@gmail.com>
Signed-off-by: Ali Caglayan <alizter@gmail.com>
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter Alizter force-pushed the ps/rr/nat__least_common_multiple branch from b96878d to 7b42942 Compare March 13, 2025 15:49
@Alizter Alizter merged commit 88577b8 into HoTT:master Mar 13, 2025
22 checks passed
@Alizter Alizter deleted the ps/rr/nat__least_common_multiple branch March 13, 2025 16:27
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.

None yet

2 participants