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

Use same arguments to printf format string for both radians and degre… #2453

Merged
merged 2 commits into from Nov 28, 2020
Merged

Use same arguments to printf format string for both radians and degre… #2453

merged 2 commits into from Nov 28, 2020

Conversation

ghost
Copy link

@ghost ghost commented Nov 25, 2020

…es in output by cct

Currently the output of the cct utility is different between radians
and degrees (as expected by cct), because of a bug in cct:

$ printf "1 2\n" | cct -z 0 -t 0 +proj=pipeline +step +proj=unitconvert +xy_in=deg +xy_out=rad
1.0000000000 2.0000000000 0.0000 0.0000

$ printf "1 2\n" | cct -z 0 -t 0 +proj=pipeline +step +proj=unitconvert +xy_in=deg +xy_out=deg
1.0000 2.0000 0.0000 0.0000

The arguments to the printf format string are as follows:

  • radians: width 14, precision 10
  • degrees: width 13, precision 4 (this is by mistake. bug!)

After the suggested fix has been applied, output will be the same for
both radians and degrees:

$ printf "1 2\n" | cct -z 0 -t 0 +proj=pipeline +step +proj=unitconvert +xy_in=deg +xy_out=rad
1.0000000000 2.0000000000 0.0000 0.0000

$ printf "1 2\n" | cct -z 0 -t 0 +proj=pipeline +step +proj=unitconvert +xy_in=deg +xy_out=deg
1.0000000000 2.0000000000 0.0000 0.0000

The cause of the bug is that cct does test if it "has radians to output",
but "neglects" to test if it "has degrees to output", resulting in using
different arguments to the printf format string in the latter case.

The fix makes cct test if it "has either radians or degrees to output".

Verified against v7.2.0

  • Closes #xxxx
  • Tests added
  • Added clear title that can be used to generate release notes
  • Fully documented, including updating docs/source/*.rst for new API

…es in output by cct

Currently the output of the cct utility is different between radians
and degrees (as expected by cct), because of a bug in cct:

$ printf "1 2\n" | cct -z 0 -t 0 +proj=pipeline +step +proj=unitconvert +xy_in=deg +xy_out=rad
  1.0000000000    2.0000000000        0.0000        0.0000

$ printf "1 2\n" | cct -z 0 -t 0 +proj=pipeline +step +proj=unitconvert +xy_in=deg +xy_out=deg
       1.0000         2.0000        0.0000        0.0000

The arguments to the printf format string are as follows:

 * radians: width 14, precision 10
 * degrees: width 13, precision  4 (this is by mistake. bug!)

After the suggested fix has been applied, output will be the same for
both radians and degrees:

$ printf "1 2\n" | cct -z 0 -t 0 +proj=pipeline +step +proj=unitconvert +xy_in=deg +xy_out=rad
  1.0000000000    2.0000000000        0.0000        0.0000

$ printf "1 2\n" | cct -z 0 -t 0 +proj=pipeline +step +proj=unitconvert +xy_in=deg +xy_out=deg
  1.0000000000    2.0000000000        0.0000        0.0000

The cause of the bug is that cct does test if it "has radians to output",
but "neglects" to test if it "has degrees to output", resulting in using
different arguments to the printf format string in the latter case.

The fix makes cct test if it "has either radians or degrees to output".

Verified against v7.2.0
@rouault
Copy link
Member

rouault commented Nov 27, 2020

would you mind adding a test case in test/cli/testcct and test/cli/testcct_out.dist ?

@ghost
Copy link
Author

ghost commented Nov 27, 2020 via email

@rouault
Copy link
Member

rouault commented Nov 27, 2020

I have modified the files and attached them to this message. (testcct and testcct_out.dist)

Modify testcct, and adjust testcct_out.dist accordingly (you can use the generated file test/cli/testcct_out to refresh testcct_out.dist). Make sure that "make check" passes, and then git commit the files and git push

@ghost
Copy link
Author

ghost commented Nov 28, 2020 via email

@kbevers
Copy link
Member

kbevers commented Nov 28, 2020

I am sorry, I cannot get this working.

It seems you've been working on different branches, which won't work. The following should work:

git checkout fix
git cherry-pick f4f1b2
git push

The first line make sure that you are working on the branch behind this pull request. The second line grabs your commit in the fix2 branch (which your other pull request is based of) and adds it to the fix branch. Last line of course pushes the updated branch to GitHub. The pull request will be updated once the updated branch is pushed - you don't need to create yet another pull request.

On request more tests have been added verifying the output of
the cct utility.

cct has an option (-d) which overrides the default value for
the number of decimals (precision) that are printed when cct
outputs the value of a coordinate.

By default the arguments to the printf format string will be:

 * radians, degrees: width 14, precision 10
 * meters: width 13, precision 4 [1]

[1] height will be shown with a width of 12.

Note: the 4th "coordinate", time, will always be shown with
a precision of 4 decimals (and width 12).

When overridden (using the -d option) all coordinates, but
not time, will be shown with the specified decimals.
@ghost
Copy link
Author

ghost commented Nov 28, 2020 via email

@rouault rouault merged commit 24c74a4 into OSGeo:master Nov 28, 2020
@rouault
Copy link
Member

rouault commented Nov 28, 2020

Commits squashed and merged. Thanks

Cherry-picked in 7.2 branch

rouault pushed a commit that referenced this pull request Nov 28, 2020
…es in output by cct (#2453)

Currently the output of the cct utility is different between radians
and degrees (as expected by cct), because of a bug in cct:

$ printf "1 2\n" | cct -z 0 -t 0 +proj=pipeline +step +proj=unitconvert +xy_in=deg +xy_out=rad
  1.0000000000    2.0000000000        0.0000        0.0000

$ printf "1 2\n" | cct -z 0 -t 0 +proj=pipeline +step +proj=unitconvert +xy_in=deg +xy_out=deg
       1.0000         2.0000        0.0000        0.0000

The arguments to the printf format string are as follows:

 * radians: width 14, precision 10
 * degrees: width 13, precision  4 (this is by mistake. bug!)

After the suggested fix has been applied, output will be the same for
both radians and degrees:

$ printf "1 2\n" | cct -z 0 -t 0 +proj=pipeline +step +proj=unitconvert +xy_in=deg +xy_out=rad
  1.0000000000    2.0000000000        0.0000        0.0000

$ printf "1 2\n" | cct -z 0 -t 0 +proj=pipeline +step +proj=unitconvert +xy_in=deg +xy_out=deg
  1.0000000000    2.0000000000        0.0000        0.0000

The cause of the bug is that cct does test if it "has radians to output",
but "neglects" to test if it "has degrees to output", resulting in using
different arguments to the printf format string in the latter case.

The fix makes cct test if it "has either radians or degrees to output".
@rouault rouault added this to the 7.2.1 milestone Nov 28, 2020
@ghost
Copy link
Author

ghost commented Nov 28, 2020 via email

@rouault
Copy link
Member

rouault commented Nov 28, 2020

your last modification of testcct: 64,65c64,65 < echo "3541657.3778 948984.2343 5201383.5231 2020.5" >> a < echo "3541658.0000 948985.0000 5201384.0000 2020.5" >> b ---
echo "3541657.3778 948984.2343 5201383.5231 2020.5" > a echo "3541658.0000 948985.0000 5201384.0000 2020.5" > b
got lost somehow, I think ...

I don't see it as lost, once merged into master. See https://github.com/OSGeo/PROJ/blob/master/test/cli/testcct#L63
As your branch was created before my yesterday's ">>a" ->">a" change, when you look at the state of the file in this pull request, my change doesn't appear. But once your changes have been merged, as they don't conflict with my change, everything is fine.
Basically a pull request is a "diff", based on the state of a branch at the time it was forked, but on merging, the "diff" is applied on the latest state of the branch into which your merge.

@ghost ghost deleted the fix branch November 28, 2020 13:59
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.

2 participants