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

Add PermuteDims transformation #89

Merged
merged 4 commits into from
Mar 11, 2024
Merged

Conversation

adrhill
Copy link
Collaborator

@adrhill adrhill commented Feb 29, 2024

Adds PermuteDims, a thin wrapper around permutedims for use on ArrayItems.

This should help with Flux compatibility (#56):
HWC to WHC conversions can be implemented by adding PermuteDims(2, 1, 3) to a pre-processing pipeline.

@darsnack
Copy link
Member

darsnack commented Mar 9, 2024

ImageToTensor already does permute the dimensions to get the channel last. We should change that so that it also permutes width/height. We can still add this transformation, because it could be useful in other contexts.

Why not use PermuteDimsArray directly to avoid copying? You can see how ImageToTensor does it as reference.

@adrhill
Copy link
Collaborator Author

adrhill commented Mar 9, 2024

Why not use PermuteDimsArray directly to avoid copying?

Good point, addressed in 3e44d1d.

ImageToTensor already does permute the dimensions to get the channel last. We should change that so that it also permutes width/height.

Wouldn't this be considered breaking? We could introduce more explicit ImageToWHCTensor and ImageToHWCTensor transformations and deprecate ImageToTensor.

@darsnack
Copy link
Member

Wouldn't this be considered breaking? We could introduce more explicit ImageToWHCTensor and ImageToHWCTensor transformations and deprecate ImageToTensor.

True, but do we really need the explicit versions if we have PermuteDims? For now, why don't we throw a deprecation warning in ImageToTensor with instructions how to get HWC in the future using (fixed) ImageToTensor + PermuteDims?

I think it's more friendly to not have the user worrying HW or WH in the long term API. An alternative would be to have explicit forms and let ImageToTensor default to one.

@adrhill
Copy link
Collaborator Author

adrhill commented Mar 11, 2024

Sounds good. I will open a separate PR for changes to ImageToTensor.
I think this one is ready to review/merge.

@darsnack
Copy link
Member

Sounds good, thanks!

@darsnack darsnack merged commit 8a39938 into FluxML:master Mar 11, 2024
6 checks passed
@adrhill adrhill deleted the ah/flip-hw branch March 11, 2024 18:21
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