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

Update horner.rst #3964

Merged
merged 3 commits into from
Dec 5, 2023
Merged

Update horner.rst #3964

merged 3 commits into from
Dec 5, 2023

Conversation

Jochem-L
Copy link
Contributor

No description provided.

@busstoptaktik
Copy link
Member

@Jochem-L Thanks for following up on this. I cannot find time to review this PR within a reasonable time frame, but I think we should ask for comments from @georgeouzou, since he is the latest person to touch the Horner docs in any substantial way, in connection with his introduction of invertible polynomiums in #3133 and #3217 last year.

Unfortunately, I cannot add @georgeouzou as suggested reviewer - perhaps because one needs commit rights in order to review? But that would not prevent him from chiming in here in the comments

@georgeouzou
Copy link
Contributor

Hi there! After checking the changes i have the following comments:

  • The tables are nice to have but they contain x and y whereas the equations above use U and V instead. We could use either but we should be consistent.
  • The indexing on the u and v parameters was (row, column). Now it seems switched to (column, row) which is in contrast to the added parameter tables.

@busstoptaktik
Copy link
Member

@georgeouzou you may not be following the PROJ mailing list anymore, so for your information, the reasoning behind some of the material @Jochem-L provides has been elaborated somewhat on in a thread over at https://lists.osgeo.org/pipermail/proj/2023-November/011166.html

@Jochem-L
Copy link
Contributor Author

@georgeouzou, Thanks for your great comments.

  • I will change the x and y in de parameter tables to U and V, that seems the easiest way to keep it consistent.
  • The problem with the indexing is the confusing convention in math to order coordinates differently than matrix indices.
    • In formula (1), the indexing is UV;
    • In the formula after (3), the indexing was VU. I suggested to change it to UV.
    • In the parameter description, the indexing was VU for U, and UV for V (and starting with 1 instead of 0). I suggested to make it to UV for both (and starting with 0).
    • For the parameter tables, I suggested the indexing UV with increasing U (x) horizontally and increasing V (y) vertically, which makes sense for coordinates, but indeed implies the col,row which is odd. I suggest to solve it by keeping the indexing UV everywhere and transposing the parameter tables to make them row,col.
The order of coefficients u_{i,j} is (example for degree 3):

+----+------+------+------+------+
|    | V⁰   | V¹   | V²   | V³   |
+----+------+------+------+------+
| U⁰ | 1st  | 5th  | 8th  | 10th |
+----+------+------+------+------+
| U¹ | 2nd  | 6th  | 9th  | –    |
+----+------+------+------+------+
| U² | 3rd  | 7th  | –    | –    |
+----+------+------+------+------+
| U³ | 4th  | –    | –    | –    |
+----+------+------+------+------+

The order of coefficients v_{i,j} is (example for degree 3):

+----+------+------+------+------+
|    | V⁰   | V¹   | V²   | V³   |
+----+------+------+------+------+
| U⁰ | 1st  | 2nd  | 3rd  | 4th  |
+----+------+------+------+------+
| U¹ | 5th  | 6th  | 7th  | –    |
+----+------+------+------+------+
| U² | 8th  | 9th  | –    | –    |
+----+------+------+------+------+
| U³ | 10th | –    | –    | –    |
+----+------+------+------+------+

Does that solve the inconsistencies you found?

@georgeouzou
Copy link
Contributor

@Jochem-L all changes seem consistent and correct!

@busstoptaktik
Copy link
Member

@Jochem-L so once you have updated the PR to reflect your comments above, I'm ready to merge the material

@busstoptaktik busstoptaktik added this to the 9.4.0 milestone Dec 5, 2023
Copy link
Member

@busstoptaktik busstoptaktik left a comment

Choose a reason for hiding this comment

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

Looks good to me

@busstoptaktik busstoptaktik merged commit decf6af into OSGeo:master Dec 5, 2023
2 checks passed
@rouault
Copy link
Member

rouault commented Dec 5, 2023

The backport to 9.3 failed:

The process '/usr/bin/git' failed with exit code 128
stderr
error: commit a377a470ec807acb0c605c8ad587b33869aa2917 is a merge but no -m option was given.
fatal: cherry-pick failed

stdout
[backport-3964-to-9.3 027dbcc0] Update horner.rst
 Author: Jochem-L <[email protected]>
 Date: Fri Nov 24 12:15:51 2023 +0100
 1 file changed, 34 insertions(+), 6 deletions(-)

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-9.3 9.3
# Navigate to the new working tree
cd .worktrees/backport-9.3
# Create a new branch
git switch --create backport-3964-to-9.3
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick 0025bf0a0ca21ba7fa6e8daa740438f63d6ca1f9,a377a470ec807acb0c605c8ad587b33869aa2917,91b4bf7d725f0f94f8f580e2383b12a38885e486
# Push it to GitHub
git push --set-upstream origin backport-3964-to-9.3
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-9.3

Then, create a pull request where the base branch is 9.3 and the compare/head branch is backport-3964-to-9.3.

mwtoews pushed a commit that referenced this pull request Dec 5, 2023
Update horner.rst with improved description of polynomium coefficient order
@mwtoews
Copy link
Member

mwtoews commented Dec 5, 2023

manually cherry-picked to 9.3 with 931c44a

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.

5 participants