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

transform update for generic transforms #60

Merged
merged 1 commit into from
Jul 11, 2023
Merged

Conversation

tdk-meta
Copy link
Contributor

Looking for Comments, Thanks

Copy link
Member

@xd009642 xd009642 left a comment

Choose a reason for hiding this comment

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

Only one comment, re docs, code all looks good 🎉

return transform;
}

pub struct AffineTransform {
Copy link
Member

Choose a reason for hiding this comment

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

I think this module is underdocumented so probably worth adding some either module level docs or type docs on the structs and public methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Finally got around to updating this, added a bunch of comments, let me know what you think.

@tdk-meta tdk-meta force-pushed the master branch 3 times, most recently from 18c541d to 9fe43f0 Compare July 5, 2023 17:00


pub fn transform_from_2dmatrix(in_array: Array2<f64>) -> AffineTransform {
/// converts a matrix into an equivalent Transform
Copy link
Member

Choose a reason for hiding this comment

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

Doc comments like this should be above the function signature.

}

pub struct AffineTransform {
/// Represents a linear transform of either size 2x2
Copy link
Member

Choose a reason for hiding this comment

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

Ditto, I imagine this comment should be above the struct? The idea is these should show up for users when they look at the docs (so run cargo doc and have a peruse to see how it looks)

@tdk-meta
Copy link
Contributor Author

@xd009642 happy to hear your feedback on this next round of comment polish. Also made ComposedTransform public

@xd009642
Copy link
Member

The doc comments still seem to be below the function signatures instead of above them?

@tdk-meta
Copy link
Contributor Author

Gah.. bad commit, sorry, should be working now.

impl Display for TransformError {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
InvalidTransform => return write!(f, "invalid transform" ),
Copy link
Member

Choose a reason for hiding this comment

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

I think here you should do Self::InvalidTransform that might fix CI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Took TransformerError::InvalidTransform to pass tests... hopefully that works.

@xd009642
Copy link
Member

Okay just a cargo fmt and then we should be good

Copy link
Member

@xd009642 xd009642 left a comment

Choose a reason for hiding this comment

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

LGTM

@xd009642 xd009642 merged commit bf7c834 into rust-cv:master Jul 11, 2023
5 of 6 checks passed
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