Skip to content

What is the purpose of from map in the Expansion Algorithm? #478

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

Closed
timothee-haudebourg opened this issue Apr 19, 2020 · 5 comments · Fixed by #481
Closed

What is the purpose of from map in the Expansion Algorithm? #478

timothee-haudebourg opened this issue Apr 19, 2020 · 5 comments · Fixed by #481

Comments

@timothee-haudebourg
Copy link

The Expansion Algorithm requires a boolean from map variable as input. Its value is never changed across the expansion algorithms and is always set to false. There are only 3 occurrences of this variable in the whole API document. It doesn't seem to be used.

@gkellogg
Copy link
Member

Step 13.8.3.6 of the Expansion Algorithm should pass from map as true when invoking the algorithm recursively. This seems to have gotten lost in some update.

As noted in the algorithm introduction, it is used to control reverting to a previous context, which can interfere with the semantics of index- id- and type-maps.

Removing it from my implementation causes expand#tc013 and expand#tm008 to fail, along with the toRdf versions of these tests.

@iherman
Copy link
Member

iherman commented Apr 24, 2020

This issue was discussed in a meeting.

  • RESOLVED: Add back from_map to 13.8.3.6 to recursive call for scoped contexts per api #478
View the transcript Rob Sanderson: #478
Gregg Kellogg: We have a flag “from map” that was not used, but it is actually used, but got lost. It is required for inherited scoped context. If an implementation does not use this, it can not pass the tests. So this would require restoring the use of this flag to fix this.
Proposed resolution: Add back from_map to 13.8.3.6 to recursive call for scoped contexts per api #478 (Rob Sanderson)
Gregg Kellogg: +1
Rob Sanderson: +1
Ruben Taelman: +1
Benjamin Young: +1
Pierre-Antoine Champin: +1
Resolution #4: Add back from_map to 13.8.3.6 to recursive call for scoped contexts per api #478

gkellogg added a commit that referenced this issue Apr 27, 2020
…e from map parameter to properly manage reverting active contexts.

Fixes #478.
@gkellogg
Copy link
Member

@timothee-haudebourg This is fixed in #481. Please let us know if that satisfies your concern.

@timothee-haudebourg
Copy link
Author

timothee-haudebourg commented Apr 28, 2020

It's fine for the text, although it would be nice to have a test that catches the correct use of from map. I've been able to pass all the expansion tests without ever using this variable.

@gkellogg
Copy link
Member

In my implementation, if I don't pass true for from map from the Create Term Definition algorithm, tests c013 and m008 fail.

gkellogg added a commit that referenced this issue Apr 29, 2020
…e from map parameter to properly manage reverting active contexts.

Fixes #478.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants