-
Notifications
You must be signed in to change notification settings - Fork 18
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
Paths and other updates #78
Conversation
…ges made. Removes the explicit universal and existential quantifiers. Improves some use of ReSpec support for definitions and other linking.
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.
Doubtless, I've missed some things ... and I only looked at the changed lines here.
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
… the example content.
…untant information on IRI resolution from the Grammar section.
Work continues. This has turned into a larger update than I had originally intended (see commit comments), I hope I'm not overstepping my mandate. We need to describe verbs more generally, and have a resolution section in the EBNF Grammar section. My thought is to describe things that are beyond Turtle as transformations to an equivalent Turtle structure (where feasible). We'll need more on parsing Graphs, and need to decide if we're changing from the use of "Formulae" to "Formulas"; the Team Submission used "Formulae" as the plural of "Formulas". |
The prior path algorithm didn't seem wholly correct to me (or perhaps I misunderstood it). Some terms were also unclear IMO (such as |pred|). E.g., the result from recursively calling the algorithm wouldn't be used as subject (`!`) / object (`^`) for an emitted statement, rather, it would be either the initial |pathItem| or the previously created |B|
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.
See commit e06a0b6 (i.e. on how I think the path algorithm should work)
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
I have to say, I don't find your version of the algorithm clearer; the recursive nature of what I proposed I find easier to understand, but perhaps there's a middle ground. Stylistically, it's good form to reduce line lengths (for the reasons @TallTed stated) and use "Otherwise" for the else-side of a clause. If the concern is for correctness, then we should also re-consider the "Paths" section, which informally defines the same transitions as re-writing the input. So that My own processor does not process paths in reverse, and implements something closer to what you state, but as a description, reverse processing seems to be easier to follow, and the intent is the results, not the method. Describing it as a forward process becomes more cumbersome, even if this is the AST generated by a parser. |
@william-vw --
Would you please make a complete PR from your fork, so that the preview of your fork can be compared to the preview of @gkellogg's fork? Or make a change suggestion on @gkellogg's PR, so that it's (somewhat) more easily seen where the differences in the source are? |
@gkellogg It's not an issue of clarity but rather correctness.. (But, as said, I could be wrong since some terms are unclear to me, e.g., |pred|, which does not occur in the grammar) As I understood it, given the following:
Then I believe the recursive call would return the last
Instead of
Honestly I don't think a recursive process is the best representation here, since the next bnode would never be utilized in the prior emitted statement. E.g., for
Should yield
And not
Note that the first item in the path should also be treated differently: |
Following my algorithm, you would get the following:
As the path was replaced with
This is not the a characteristic of the recursive algorithm, but of the misappropriation of obj as the result, and not
As the path was replaced with So, the resulting triples are: @TallTed said:
This would be useful: you can actually make a PR to the branch that this PR is based. Doing it through the suggestion mechanism is probably possible, but challenging. I'll just lay them out here in Markdown. Gregg's (corrected) algorithm
(Given the commonality in steps 4. and 5., the creation of William's algorithm:
(Also, note that |
@gkellogg Ahhh .. I understand how your algorithm works now. (This is why I've learned to add disclaimers such as "as far as I understood"!) Honestly it wasn't clear to me until you gave the example. I think this is the main thing that tripped me up:
As I mentioned, |pred| wasn't clear to me; and I thought |path| referred to the last element in the production rule. So IIUC, |path| comprises the resource path with length n-1 (not including the last path item) and |pred| represents the last path item (?) The hint "from the last occurrence of the directional indicator |dir|" was insufficient for me, it seems. I only considered recursion in a "forward" direction, not "backward", hence my comment about appropriateness. (Unsure whether the fixed issue with |obj| would have made a difference in my understanding, really.) I do appreciate the elegance of the algorithm, but it's a bit unfortunate that it doesn't translate as easily to an event-based parser (i.e., a listener vs. a visitor) since the whole AST is not (yet) available. I think the following would be clearer to me (no ReSpec formatting at this point):
Perhaps both algorithms can be given - with a toggle "recursive" vs. "iterative" :-) @TallTed Hope that Gregg's markdown version clarifies the differences better. |
The algorithms are definitely much more understandable with the examples. I think such should be included in the doc! I don't have a strong feeling for/against either iterative or recursive processing. If both are viable (which I cannot yet discern), I would suggest they both be included, each with appropriate examples -- optimally with the same example data being handled by both iterative or recursive algorithms -- as it seems likely to me that different implementations/environments would be better served by each. |
Good suggestion.
I think it would be useful to specify alternates to the algorithm, as that would make it clear that there is no one right way to implement it and only the result matters. It did occur to me, in the middle of the night, that we've underspecified how these algorithms relate to being in subject, predicate, or object positions, since the necessary resource to use varies. I had it in mind to work more closely with the Turtle curSubject and curObject descriptions, but that looks like pulling on a thread that would have no end. I'll make an update that includes both algorithms, along with examples of execution for our sample paths, including in at least subject, object and maybe list member contexts, as appropriate. |
@william-vw I took some liberties with your algorithm, which I believe are isomorphic and, IMHO, easier to follow. I also included evaluation examples for each. See what you think. |
I don't know if you want(ed) to do this in this PR, but in any case — I would have said Formulae, but Grammar Monster and Google's Ngram Viewer provide strong justification for "Formulas". |
We discussed this on a call the other week. It wasn't intentional to move from "Formulae" to "Formulas", although both are correct. "Formulae" is, IMHO, more prosaic, but it is what was used in the original Team Submission, and changing it would seem gratuitous at this point. For example, documentation in my implementation has long used "formulae". We'll do a more comprehensive update to normalize usage 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.
I think the updates are great. I suggested some minor changes on the right-to-left algorithm, and some larger ones on the left-to-right one (see comments for details).
(<var>B<sub>n</sub></var> <var>item<sub>n</sub></var> <var>B<sub>n-1</sub></var>).</li> | ||
</ol> | ||
|
||
<aside class="example" |
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.
to be edited if we reach a consensus about updated iterative algorithm
Co-authored-by: William Van Woensel <william.van.woensel@gmail.com>
Co-authored-by: William Van Woensel <william.van.woensel@gmail.com>
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.
some minor suggestions - looks good
spec/index.html
Outdated
<ul> | ||
<li>Step 1 does not apply, | ||
as <var>dir<sub>0</sub></var> is not `null`.</li> |
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.
as <var>dir<sub>0</sub></var> is not `null`.</li> | |
as |n| is `0`.</li> |
Co-authored-by: William Van Woensel <william.van.woensel@gmail.com>
Includes a number of changes outside of the paths section to better use ReSpec and remove some archaic concepts.
Preview | Diff