-
-
Notifications
You must be signed in to change notification settings - Fork 4
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
Precedence for Directed and Undirected Edge #103
Merged
+182
−307
Merged
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
54a2c3a
fix precedence for Directed and Undirected Edge
mmatera da9bd6d
black and remove comment
mmatera 90aef9b
Merge remote-tracking branch 'origin/master' into precedence_Undirect…
mmatera 306a798
Improving docstrings (#105)
mmatera 242eb0a
adding rockys tests
mmatera f942868
adding operational tests
mmatera faec1fd
merge
mmatera 2433785
Merge remote-tracking branch 'origin/precedence_Undirected_and_Direct…
mmatera f19e4cb
black
mmatera 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
Oops, something went wrong.
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.
The precedence order of these symbols is tested here. Of course, other tests can be added.
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.
This test is extremely opaque. (BTW the one for TestConstrained is also that way.)
I was hoping for something operational or something a user might experience, rather than checking that the value in this table is less than another value.
Setting operator precedence determines whether we have parenthesis or not. So in a chain of direct/directed operators, we should see in full form the grouping change, and when there are other operators around them, then either parenthesis or the way calls are nested change.
I seem to recall that there were tests around that worked that way. This is testing the effect of the precedence values on the way parsing happens.
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.
OK, but this is what we can check without bitting our tails: we cannot deduce what the precedence order is for a given pair of operators: we need to look at a reference. The parser or the formatter of Mathics-core could show the right behavior without looking at the tables. And even if they do, checking that their behavior is consistent with the information in the table is not a warranty that this information is right, because it was produced from it: What we are checking is the parser and the formatter, not the accuracy of the information.
What I think it makes more sense is to check if the precedence order in WMA is consistent with its own behavior, and handle the inconsistencies by slightly adjust the values that we store. This is why I wrote this kind of operational test in a WL module. Then, if someone has doubts about one precedence value in the table, can go to wolframscript and check it out. Of course, we can also use it over mathics-core.
Maybe it worth to put this in a comment on the pytest module, but it would go in another PR.
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.
The initial commentary for this PR shows WMA code that was run demonstrating why the precedence needs to be changed.
I want the tests to reflect that by doing the same thing. In general, if one observes that something is wrong because when I run something I get this, then fixing that should run the demonstration to show this is now addressed.
In the future, developers are more likely to encounter the pytest code rather than some PR commentary. The PR commentary is not checked in a simple-minded and easy-to-understand way.
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.
OK, but the comments have the only purpose to converge to some criteria about what value must be assigned. Once these criteria can be clearly written, the comments can go out. I started to do that in #105, over this.
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.
#105 is fine (with one small removal of slight vagueness).
This PR changes the operator precedence numerical value of
DirectedEdgeandUndirectedEdge. I am asking that a test be written to demonstrate why the operator precedence change in value is needed and to check that the operational effect we want to achieve persists in the future.