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

ColorJitter: ColorJitterView -> MappedArray #93

Merged
merged 1 commit into from
Aug 9, 2021

Conversation

barucden
Copy link
Collaborator

@barucden barucden commented Aug 8, 2021

The specific struct ColorJitterView, which we use for lazy augmentation in ColorJitter, is just a MappedArray. (I did not know MappedArrays.jl when implementing the operation.)

I don't think it is necessary to produce a new package version since nothing has changed, only the code is reduced.

The specific struct `ColorJitterView`, which we use for lazy
augmentation in ColorJitter, is just a MappedArray.
Copy link
Collaborator

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

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

I didn't introduce it because MappedArrays had some inference issue, which was fixed just yesterday :)

Generally, I wish we could move more operation implementation out of this package, and focus on pipeline optimization and API specification. So this is a good change from my point of view.

@barucden
Copy link
Collaborator Author

barucden commented Aug 8, 2021

Funny coincidence then! 😄

I wish we could move more operation implementation out of this package

ColorJitter goes against. Do you think it would be reasonable to move the operation to ImageContrastAdjustment.jl? If so, I can start an issue there and work on a PR.

@johnnychen94
Copy link
Collaborator

Opening an issue and consider a generic implementation definitely have benefits and usually make the code more extensible. Maybe the question is, can we make ColorJitter support more predefined operations in ImageContrastAdjustment?

I don't play deep learning recently so maybe you get more idea of whether this kind of generalization is wanted.

@johnnychen94
Copy link
Collaborator

At least for the current ColorJitter implementation, it's simple enough to be hold in this package.

@barucden
Copy link
Collaborator Author

barucden commented Aug 9, 2021

IMHO, ColorJitter should do what it does now, as simple as it is. My reasoning was that it is a common image adjustment method (which can also be defined by histogram transformation) so maybe it would be a good fit for ImageContrastAdjustment.jl.

I think we should definitely support some of the operations defined in ImageContrastAdjustment.jl. I’ve seen a few of them being used in deep learning. But I guess they should be a separate operations, not ColorJitter.

@barucden barucden merged commit a588324 into Evizero:master Aug 9, 2021
@barucden barucden deleted the simplify-color-op branch August 9, 2021 10:47
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