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

PLT-5369 - Simplification of some linorder instances #169

Merged
merged 5 commits into from
Apr 19, 2023

Conversation

hrajchert
Copy link
Collaborator

@hrajchert hrajchert commented Mar 17, 2023

This PR is marked as Draft until PR 168 is merged.

After applying the small fixes in PR 168 regarding the linorder instances, I saw that I could simplify them even further.

@hrajchert hrajchert marked this pull request as draft March 17, 2023 19:13
@hrajchert hrajchert force-pushed the hrajchert/scp-3505-small-fixes branch 2 times, most recently from 81e76e4 to 2f1fcc7 Compare March 23, 2023 14:08
@hrajchert hrajchert force-pushed the hrajchert/linorder-simplification branch from ee8c6a7 to 4f4d5f5 Compare March 28, 2023 18:12
@hrajchert hrajchert force-pushed the hrajchert/scp-3505-small-fixes branch from 5694151 to 97fac7d Compare April 7, 2023 14:51
@hrajchert hrajchert force-pushed the hrajchert/linorder-simplification branch from 4f4d5f5 to 22e262d Compare April 7, 2023 15:01
Base automatically changed from hrajchert/scp-3505-small-fixes to master April 14, 2023 14:49
@hrajchert hrajchert marked this pull request as ready for review April 14, 2023 14:49
@hrajchert hrajchert requested a review from palas April 14, 2023 14:51
Copy link
Collaborator

@palas palas left a comment

Choose a reason for hiding this comment

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

Thanks. It looks good, except for the unnecessary case statements, which I don't see the point

isabelle/generated/SemanticsGuarantees.hs Outdated Show resolved Hide resolved
isabelle/generated/SemanticsGuarantees.hs Outdated Show resolved Hide resolved
isabelle/generated/SemanticsGuarantees.hs Outdated Show resolved Hide resolved
isabelle/generated/SemanticsGuarantees.hs Outdated Show resolved Hide resolved
isabelle/generated/SemanticsGuarantees.hs Outdated Show resolved Hide resolved
@hrajchert hrajchert requested a review from palas April 14, 2023 21:36
Copy link
Collaborator

@palas palas left a comment

Choose a reason for hiding this comment

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

The simplification is very good, but I don't like the replacing funs with definitions, funs are so much easier to work with



definition "a \<le> b = less_eq_BS a b"
definition "a \<le> b = less_eq_BS' (innerListByteString a) (innerListByteString b)"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Like we discussed, it is better to have them as pattern matching and as fun rather than point-free and definition, because when using Isabelle interactively it expands worse, and also because definitions do not simplify by default whereas fun does, and fun generates many more lemmas for free

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If I try to do it with fun directly it fails to prove termination automatically.
Screen Shot 2023-04-18 at 18 28 51

I agree that this one in particular is the ugliest of the current definitions, but is still a step forward from previous code

fun less_Party :: "Party \<Rightarrow> Party \<Rightarrow> bool" where
"less_Party a b = (\<not> (less_eq_Party b a))"
definition less_Party :: "Party \<Rightarrow> Party \<Rightarrow> bool" where
"less_Party a b \<longleftrightarrow> \<not> ( b \<le> a)"
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you can choose between having an abstract predicate that is not executable and something that is just a normal computable function, choosing the abstract predicate to save a parenthesis really isn't worth it. If it was a lemma, then I would understand, but it is just a function

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure what you mean with something abstract that is not executable. This is exported as code as following:
Screen Shot 2023-04-18 at 18 09 00

The "simp problem" mentioned in a previous comment is easily solved here by the line directly below:

declare less_Party_def [simp add]

which adds the definition to the simp set. I'm using the principle of least power. If you can solve a problem two different ways, and one of them is simpler, the simpler tool should be used. I assume that is the reason why definition, primrec, fun and function exist in the first place, and a simple search by text in the Isabelle source code yields

7526 definition
819 primrec
2241 fun
1460 function


definition "a < b = less_BS a b"
definition less_ByteString :: "ByteString \<Rightarrow> ByteString \<Rightarrow> bool" where
"less_ByteString a b \<longleftrightarrow> \<not> (b \<le> a)"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here

)"

definition less_ChoiceId :: "ChoiceId \<Rightarrow> ChoiceId \<Rightarrow> bool" where
"less_ChoiceId a b \<longleftrightarrow> \<not> (b \<le> a)"
Copy link
Collaborator

Choose a reason for hiding this comment

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

And here

@hrajchert hrajchert merged commit a277b2e into master Apr 19, 2023
@hrajchert hrajchert deleted the hrajchert/linorder-simplification branch April 19, 2023 14:14
@palas palas changed the title Simplification of some linorder instances PLT-5369 - Simplification of some linorder instances Apr 24, 2023
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