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.

Sign up for GitHub

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

Jump to bottom

Unclear explanation of namespace calculation when IgnoreMaxNamespace is set #133

Closed
ivan-gavran opened this issue Mar 14, 2023 · 0 comments · Fixed by #134
Closed

Unclear explanation of namespace calculation when IgnoreMaxNamespace is set #133

ivan-gavran opened this issue Mar 14, 2023 · 0 comments · Fixed by #134
Assignees
@ivan-gavran
Labels
audit Issue uncovered during Informal audit documentation Improvements or additions to documentation

Comments

@ivan-gavran
Copy link
Collaborator

The section which explains how to calculate the namespace range when the parameterIgnoreMaxNamespace can be confusing. (here).
In particular, the part

This is achieved by taking the maximum value among the namespace IDs available in the range of node's left and right children (i.e., n.maxNs = max(l.minNs, l.maxNs , r.minNs, r.maxNs)), which is not equal to the maximum possible namespace ID value.

could be read as suggesting that the calculated n.maxNs is necessarily no equal to the maximum possible namespace ID value.
@ivan-gavran ivan-gavran mentioned this issue Mar 14, 2023
Merged
5 tasks
@rootulp rootulp added the documentation Improvements or additions to documentation label Mar 14, 2023
evan-forbes added a commit that referenced this issue Mar 15, 2023
@ivan-gavran @evan-forbes
Reworded explanation of calculating namespace when IgnoreMaxNamespace…
25711ba 
… is set (#134)

<!--
Please read and fill out this form before submitting your PR.

Please make sure you have reviewed our contributors guide before
submitting your
first PR.
-->

## Overview

<!-- 
Please provide an explanation of the PR, including the appropriate
context,
background, goal, and rationale. If there is an issue with this
information,
please provide a tl;dr and link the issue. 
-->
This PR rephrases the explanation on how the namespace range is
calculated when the parameter `IgnoreMaxNamespace` is set. If merged, it
will close #133 .

I tried to simplify sentences explaining the logic and add a precise
expression at the end of the explanation.

[rendered
document](https://github.com/ivan-gavran/nmt/blob/ivan/ignoreMaxNsExplanation/docs/nmt-lib.md)

## Checklist

<!-- 
Please complete the checklist to ensure that the PR is ready to be
reviewed.

IMPORTANT:
PRs should be left in Draft until the below checklist is completed.
-->

- [ ] New and updated code has appropriate documentation
- [ ] New and updated code has new and/or updated testing
- [ ] Required CI checks are passing
- [ ] Visual proof for any user facing features like CLI or
documentation updates
- [x] Linked issues closed with keywords

---------

Co-authored-by: Evan Forbes <42654277+evan-forbes@users.noreply.github.com>
@liamsi liamsi added the audit Issue uncovered during Informal audit label Mar 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ivan-gavran ivan-gavran

Labels
audit Issue uncovered during Informal audit documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Reworded explanation of calculating namespace when IgnoreMaxNamespace is set ivan-gavran/nmt
3 participants
@rootulp @liamsi @ivan-gavran