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

Apply fixes from -Wclippy::pedantic #13

Merged
merged 3 commits into from
Jan 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions ocrs-cli/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -219,16 +219,18 @@ fn main() -> Result<(), Box<dyn Error>> {
let detection_model_src = args
.detection_model
.as_ref()
.map(|path| ModelSource::Path(path))
.unwrap_or(ModelSource::Url(DETECTION_MODEL));
.map_or(ModelSource::Url(DETECTION_MODEL), |path| {
ModelSource::Path(path)
});
let detection_model = load_model(detection_model_src)
.file_error_context("Failed to load text detection model", detection_model_src)?;

let recognition_model_src = args
.recognition_model
.as_ref()
.map(|path| ModelSource::Path(path))
.unwrap_or(ModelSource::Url(RECOGNITION_MODEL));
.map_or(ModelSource::Url(RECOGNITION_MODEL), |path| {
ModelSource::Path(path)
});
let recognition_model = load_model(recognition_model_src).file_error_context(
"Failed to load text recognition model",
recognition_model_src,
Expand Down
2 changes: 1 addition & 1 deletion ocrs-cli/src/output.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ pub fn generate_annotated_png(args: GeneratePngArgs) -> NdTensor<f32, 3> {
let floor_point = |p: PointF| Point::from_yx(p.y as i32, p.x as i32);

// Draw line bounding rects from layout analysis step.
for line in line_rects.iter() {
for line in line_rects {
let line_points: Vec<_> = line
.iter()
.flat_map(|word_rect| word_rect.corners().into_iter())
Expand Down
8 changes: 3 additions & 5 deletions ocrs/src/detection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,11 +83,9 @@ impl TextDetector {
// Add batch dim
let image = image.reshaped([1, img_chans, img_height, img_width]);

let (in_height, in_width) = match self.input_shape[..] {
[_, _, Dimension::Fixed(h), Dimension::Fixed(w)] => (h, w),
_ => {
return Err("failed to get model dims".into());
}
let [_, _, Dimension::Fixed(in_height), Dimension::Fixed(in_width)] = self.input_shape[..]
else {
return Err("failed to get model dims".into());
};

// Pad small images to the input size of the text detection model. This is
Expand Down
9 changes: 4 additions & 5 deletions ocrs/src/layout_analysis.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,27 +92,26 @@ pub fn find_block_separators(words: &[RotatedRect]) -> Vec<Rect> {
lines.sort_by_key(|l| l.first().unwrap().bounding_rect().top().round() as i32);

let mut all_word_spacings = Vec::new();
for line in lines.iter() {
for line in lines {
if line.len() > 1 {
let mut spacings: Vec<_> = zip(line.iter(), line.iter().skip(1))
.map(|(cur, next)| {
(next.bounding_rect().left() - cur.bounding_rect().right()).round() as i32
})
.collect();
spacings.sort();
spacings.sort_unstable();
all_word_spacings.extend_from_slice(&spacings);
}
}
all_word_spacings.sort();
all_word_spacings.sort_unstable();

let median_word_spacing = all_word_spacings
.get(all_word_spacings.len() / 2)
.copied()
.unwrap_or(10);
let median_height = words
.get(words.len() / 2)
.map(|r| r.height())
.unwrap_or(10.)
.map_or(10.0, |r| r.height())
.round() as i32;

// Scoring function for empty rectangles. Taken from Section 3.D in [1].
Expand Down
2 changes: 1 addition & 1 deletion ocrs/src/layout_analysis/empty_rects.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ where
queue.push(Partition {
score: score(boundary),
boundary,
obstacles: obstacles.to_vec(),
obstacles: obstacles.clone(),
Copy link
Owner

Choose a reason for hiding this comment

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

I slightly prefer to_vec here because it makes the type more obvious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking into the code, obstacles is already made into a vec above
line 57 let mut obstacles = obstacles.to_vec();
Since is not consumed elsewhere later I think we can just use the shorthand initialization without .clone() or to_vec()

it would look like this

queue.push(Partition {
                score: score(boundary),
                boundary,
                obstacles,
            });

do you still prefer the original over this?

Copy link
Owner

Choose a reason for hiding this comment

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

You're right. The clone looks redundant. This could be made clearer by moving initialization of it inside the !boundary.is_empty() check.

});
}

Expand Down
32 changes: 17 additions & 15 deletions ocrs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,10 +95,11 @@ impl OcrEngine {
/// Returns an unordered list of the oriented bounding rectangles of each
/// word found.
pub fn detect_words(&self, input: &OcrInput) -> Result<Vec<RotatedRect>, Box<dyn Error>> {
let Some(detector) = self.detector.as_ref() else {
return Err("Detection model not loaded".into());
};
detector.detect_words(input.image.view(), self.debug)
if let Some(detector) = self.detector.as_ref() {
detector.detect_words(input.image.view(), self.debug)
} else {
Err("Detection model not loaded".into())
}
}

/// Perform layout analysis to group words into lines and sort them in
Expand Down Expand Up @@ -128,17 +129,18 @@ impl OcrEngine {
input: &OcrInput,
lines: &[Vec<RotatedRect>],
) -> Result<Vec<Option<TextLine>>, Box<dyn Error>> {
let Some(recognizer) = self.recognizer.as_ref() else {
return Err("Recognition model not loaded".into());
};
recognizer.recognize_text_lines(
input.image.view(),
lines,
RecognitionOpt {
debug: self.debug,
decode_method: self.decode_method,
},
)
if let Some(recognizer) = self.recognizer.as_ref() {
Copy link
Owner

Choose a reason for hiding this comment

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

This is OK, although if new lines were added to the happy path, I'd probably want to revert back to the early-exit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if I understand correctly, the two versions of the code operate identically and the
if let Some vs let - else construction merely moves the main body vs exception placements around. This arguably makes it clearer what this part of the code intends to do vs what it does when it fails.

Usually let else is used when there are a lot of following code, where the if let Some pattern will introduce braces and tab drift.
Of course, if we assume both operate the same it becomes a matter of choice, and the opinion of the main maintainer/developer matters more IMO

If i inadvertently introduced any change in behavior I think this should also revert back to the original.

Copy link
Owner

Choose a reason for hiding this comment

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

Usually let else is used when there are a lot of following code, where the if let Some pattern will introduce braces and tab drift.

Yes, this is one reason. The other is to logically have early-exit checks at the top of the function and then have the main body concentrate on the happy path. This also matters mainly when there are multiple early-exits. With the current body both are easy to read, and I would be fine with either.

recognizer.recognize_text_lines(
input.image.view(),
lines,
RecognitionOpt {
debug: self.debug,
decode_method: self.decode_method,
},
)
} else {
Err("Recognition model not loaded".into())
}
}

/// Convenience API that extracts all text from an image as a single string.
Expand Down
2 changes: 1 addition & 1 deletion ocrs/src/preprocess.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ fn greyscale_image<F: Fn(f32) -> f32>(
) -> NdTensor<f32, 3> {
let [chans, height, width] = img.shape();
assert!(
chans == 1 || chans == 3 || chans == 4,
matches!(chans, 1 | 3 | 4),
"expected greyscale, RGB or RGBA input image"
);

Expand Down
14 changes: 4 additions & 10 deletions ocrs/src/recognition.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,14 +157,12 @@ fn polygon_slice_bounding_rect(
let trunc_edge_start = e
.to_f32()
.y_for_x(min_x as f32)
.map(|y| Point::from_yx(y.round() as i32, min_x))
.unwrap_or(e.start);
.map_or(e.start, |y| Point::from_yx(y.round() as i32, min_x));

let trunc_edge_end = e
.to_f32()
.y_for_x(max_x as f32)
.map(|y| Point::from_yx(y.round() as i32, max_x))
.unwrap_or(e.end);
.map_or(e.end, |y| Point::from_yx(y.round() as i32, max_x));

Some(Line::from_endpoints(trunc_edge_start, trunc_edge_end))
})
Expand Down Expand Up @@ -370,7 +368,7 @@ impl TextRecognizer {
let min_width = 10.;
let max_width = 800.;
let aspect_ratio = orig_width as f32 / orig_height as f32;
(height as f32 * aspect_ratio).max(min_width).min(max_width) as u32
(height as f32 * aspect_ratio).clamp(min_width, max_width) as u32
}

// Group lines into batches which will have similar widths after resizing
Expand Down Expand Up @@ -489,11 +487,7 @@ mod tests {
// Vary the orientation of words. The output of `line_polygon`
// should be invariant to different orientations of a RotatedRect
// that cover the same pixels.
let up = if i % 2 == 0 {
Vec2::from_yx(-1., 0.)
} else {
Vec2::from_yx(1., 0.)
};
let up = Vec2::from_yx(if i % 2 == 0 { -1. } else { 1. }, 0.);
RotatedRect::new(center, up, width, height)
})
.collect();
Expand Down
4 changes: 2 additions & 2 deletions ocrs/src/test_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,12 @@ pub fn gen_rect_grid(
rect_size: (i32, i32),
gap_size: (i32, i32),
) -> Vec<Rect> {
let mut rects = Vec::new();

let (rows, cols) = grid_shape;
let (rect_h, rect_w) = rect_size;
let (gap_h, gap_w) = gap_size;

let mut rects = Vec::with_capacity((rows * cols) as usize);

for r in 0..rows {
for c in 0..cols {
let top = top_left.y + r * (rect_h + gap_h);
Expand Down