Skip to content

Conversation

@shrit
Copy link
Member

@shrit shrit commented Mar 2, 2025

Adding cropping functionality for images, in addition to the previous resizes added previously

Copy link
Member

@rcurtin rcurtin left a comment

Choose a reason for hiding this comment

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

This will be a nice addition! I think you have already said that you want to do this, but we should probably figure out a better place for the documentation about the image functionality sometime.

}
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we could also add a test that if we crop/resize a 360x360 image to 180x180, that the pixels we get are the center 180x180 of the original image? And the same for, e.g., 360x180 -> 180x180 and 180x360 -> 180x180. I can help write that test if you want, just let me know. But I am a little paranoid that we should actually check that we are getting the correct pixels as output, because it would be really easy to modify this function in the future in a way that causes these tests to pass, but actually garbles the pixel outputs.

Copy link
Member Author

Choose a reason for hiding this comment

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

nope, so if the dimensions are equal then only resize will happen no need to crop the images
Crop only happens to keep the aspect ration without having a resolution problem,
The best as you said is to show before and after it will be much easier to understand.

Copy link
Member

Choose a reason for hiding this comment

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

Right, I understand now; thanks for the clarification. Still though, if we crop and resize from 360x180 -> 180x180, then we can still check that we get the center of the image: the output pixel (x, y) should be the same as the input pixel (90 + x, y). Do you think we can add a test for that?

Copy link
Member

@rcurtin rcurtin left a comment

Choose a reason for hiding this comment

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

Sorry for the slow review on this. I didn't have a chance to look into what is failing, but at least the implementation of cropping seems just fine to me when I looked through it. I had some comments about the scope of the function---I wonder if it would be clearer to users if we kept the functionality as just cropping to the center of the image, and then allowing users to resize images with existing functionality. But maybe there is something I missed with that idea; let me know what you think.

}
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Right, I understand now; thanks for the clarification. Still though, if we crop and resize from 360x180 -> 180x180, then we can still check that we get the center of the image: the output pixel (x, y) should be the same as the input pixel (90 + x, y). Do you think we can add a test for that?

Copy link
Member Author

@shrit shrit left a comment

Choose a reason for hiding this comment

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

Looks good we can approve it

Copy link
Member

@rcurtin rcurtin left a comment

Choose a reason for hiding this comment

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

I pushed a quick change to HISTORY.md to point out that we added the feature. Everything else looks great---let's get it merged 😄

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Second approval provided automatically after 24 hours. 👍

@shrit shrit merged commit aa613f8 into mlpack:master Mar 30, 2025
15 of 17 checks passed
@shrit shrit deleted the crop branch March 30, 2025 09:29
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