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

Fixes some logical issues in matrix and vector code #51

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

microbuilder
Copy link
Member

Address some small logical and code problems identified in the issues below:

Fixes #46
Fixes #47
Fixes #48

Adds missing `break` statements in `zsl_mtx_binary_op`.

Signed-off-by: Kevin Townsend <kevin@ktownsend.com>
Fixes two small logical errors in `zsl_vec_sort` and
`zsl_vec_zte`.

Signed-off-by: Kevin Townsend <kevin@ktownsend.com>
@microbuilder
Copy link
Member Author

microbuilder commented May 28, 2023

@uLipe When you have a few minutes, do you mind doing a quick check of these and approve if you're happy with the changes? I completely missed the issues that were raised in January (thanks @ruehlchris).

@@ -372,7 +372,7 @@ int zsl_vec_zte(struct zsl_vec *v)

for (size_t g = 0; g < v->sz; g++) {
if ((v->data[g - x] >= 0.0 && v->data[g - x] < epsilon) ||
(v->data[g - x] <= 0.0 && v->data[g - x] > epsilon)) {
(v->data[g - x] <= 0.0 && v->data[g - x] > -epsilon)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick: It seems this is a saturation operation, if so I would not use less/greater and equal, since we only want to wrap around to a desired value when the variable surpasses it.

Copy link
Collaborator

@uLipe uLipe left a comment

Choose a reason for hiding this comment

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

I have a minor, not blocking, comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants