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

Func util empty arr #51

Merged
merged 11 commits into from
Jan 16, 2025
Merged

Func util empty arr #51

merged 11 commits into from
Jan 16, 2025

Conversation

InderKhera
Copy link
Collaborator

  • created tests for non-empty array to be handled
  • created tests to ensure dimensions of numpy array align with 2d grayscale array or 3d RGB array

@InderKhera InderKhera requested review from JennyonOort, hankunxiao and Arc-Celt and removed request for JennyonOort and hankunxiao January 16, 2025 03:32
Copy link
Collaborator

@Arc-Celt Arc-Celt left a comment

Choose a reason for hiding this comment

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

Well done with the utility function for numpy shape and thanks for adding the dependencies Inder! Just a few thoughts on the test:

  • I see that right now you have one error case that is a 1D array. You could try adding more expected test cases as well as edge cases as well, so that each category has more than 2 test cases. Here are some cases I thought of:
    • Expected cases: valid 2D array, and valid 3D array.
    • Edge cases: 1 pixel 2D or 3D array, arrays containing different datatypes such as float.
    • Error cases: the 1D array you wrote, empty array, zero-sized-deminsion array.
  • (Optional) As Jenny suggested, you can use pytest's decorator we learnt today pytest.mark.parametrize to have multiple test cases combined together to reduce duplication. This is how I used it for your reference.

@InderKhera
Copy link
Collaborator Author

@Arc-Celt Thanks for bringing this up, it looks like I had an issue with my push to the remote where only one of the cases showed up. I will amend this as well as use the pytest.mark.parametrize

@Arc-Celt Arc-Celt self-requested a review January 16, 2025 06:46
Copy link
Collaborator

@Arc-Celt Arc-Celt left a comment

Choose a reason for hiding this comment

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

I just ran pytest and found that the cases for test_erroneous_image_format and test_zero_size_dimensions didn't raise ValueError, which is strange because when I tried your test cases to see if they would raise the ValueError, they did it correctly. Is pytest working on your side that all the test cases pass?

@Arc-Celt Arc-Celt self-requested a review January 16, 2025 07:24
Copy link
Collaborator

@Arc-Celt Arc-Celt left a comment

Choose a reason for hiding this comment

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

Everything looks good to me now! One thing to note is that you don't need to explicitly write return None in the _input_checker function, as it will automatically return None if there is no return statement.

@InderKhera InderKhera merged commit 71a8761 into main Jan 16, 2025
2 of 4 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