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

Replaces multi-var str() concat with %-format #124

Merged
merged 4 commits into from
Oct 23, 2016

Conversation

kba
Copy link
Collaborator

@kba kba commented Oct 20, 2016

This cleans up the print_* calls in those files that redundantly cast numbers to string (as mentioned in #103)

"%s" % (str(1))
# ->
"%d" % (1)

fname+": %d" % 3
# ->
"%s: %d" % (fname, 3)

"%s" % (str(1))
->
"%d" % (1)

fname+": %d" % 3
->
"%s: %d" % (fname, 3)
@@ -286,8 +286,9 @@ if args.estrate:
terr += err
total += n
confusions += conf
print_info("%.5f"%(terr*1.0/total) + " " + terr+ " " + total + " " + args.model)
print_info("%.5f %d %d %d" % (terr*1.0/total, terr, total, args.model))
Copy link
Collaborator

Choose a reason for hiding this comment

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

args.model is a string, i.e. use %s for the last variable.

return
if scale<args.minscale:
print_error("%s: scale (%s) less than --minscale; skipping\n"%(fname,str(scale)))
print_error("%s: scale (%g) less than --minscale; skipping\n" % (fname, scale))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason you do here first a %f and not as in the subsequent lines %g? (This looks inconsistent also it may not be changing much, because the numbers for scale are normally not that large.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

%f by default is more precise, 19.493589 seemed preferable to 19.4936 to me in this case. In the other instances, I just stuck with the original formatting since I couldn't test it. I'm not a big fan of exponential format.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, let us leave it as you have here then.

return

# find columns and text lines

if not args.quiet: print_info("computing segmentation")
segmentation = compute_segmentation(binary,scale)
if amax(segmentation)>args.maxlines:
print_error(fname + ": too many lines" + str(amax(segmentation)))
print_error("%s too many lines %g" % (fname, amax(segmentation)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Colon

print_info("#" + fname)
if args.parallel<2: print_info("=== "+fname+ " " +str(i))
print_info("# %s" % (fname))
if args.parallel<2: print_info("=== %s%-3d" % (fname, i))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a space here again. This currently looks like

INFO:  === tests/testpage.png1

@@ -192,7 +192,7 @@ def process1(job):
bin = 1*(flat>args.threshold)

# output the normalized grayscale and the thresholded images
print_info(fname+" lo-hi (%.2f %.2f) angle %4.1f"%(lo,hi,angle) + comment)
print_info("%s lo-hi (%.2f %.2f) angle %4.1f %s" % (fname, lo, hi, angle, comment))
Copy link
Collaborator

@zuphilip zuphilip Oct 23, 2016

Choose a reason for hiding this comment

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

This produces sometimes double spaces, e.g.

INFO:  tests/testpage.png lo-hi (0.39 1.44) angle  0.1  no-normalization

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or is this just reserving enough space for %4.1f?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The first two spaces are because of %4.f for a value < 10. The second two spaces are fixed now.

@zuphilip
Copy link
Collaborator

This looks pretty good. I have one larger remark (which you already changed) and some small nits.

@zuphilip zuphilip merged commit 75f57fd into ocropus-archive:master Oct 23, 2016
@zuphilip
Copy link
Collaborator

Thank you very much! With this PR you solved two other issues! 🎉

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