Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[py-tx] embeded tx hash for unletterboxing #1684
[py-tx] embeded tx hash for unletterboxing #1684
Changes from 1 commit
926801e
9dba274
bfc10da
00c0b09
58e0d3d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: If you make the action
store_true
then the default is false IIRC.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(no changes needed): Do you think we should combine rotations into your generalization of preprocessing?
Alternatively, what do you think about making rotation and unletterboxing mutually exclusive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I thought that it made the most sense to combine but didn't want to jump ahead and do it beforehand but I can add it to the list of actions.
Also, would there ever be a workflow where someone may want to both check for rotation and process the image for letterboxing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only PdqSignal? Wouldn't other image perceptual hashing algorithms benefit from this?
We also generally want to avoid places where we do
isinstance(<interface_implementation>)
in preference of it being handled by the interface itself.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I thought it was specific to PdqSignal and as for why I bypassed the interface it is because they do not all have the method hash from bytes and I did not always want to create the new file with updated images bytes. Is there a way I can go around this or would it be better to always create the new file even if temporarily, and then save the output if the user passes the flag to save it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not specific to PdqSignal, and the hash_from_bytes method is itself part of a wider interface.
I think I eventually decided it was simpler to write everything to tmpfiles, which is how we ended up with the current implementation.
However, similar to the feedback I gave for --rotations, we can make our life a lot easier by having the preprocessing happen in between the file input and the
for file in self.files
.I like your idea of providing a way to pass flag to save it, but let's save that for a followup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, more specialization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Instead, in the
__init__
validate that unletterbox can only be used with photo, which will save you many future checks like this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By putting these all at the top level, we are signaling that they are part of the "official" interface for photos.
Instead, let's move this functionality into its own file in a new
/preprocessing
directory. We can addunletterbox.py
as its own module, with these 4 methods then as standalone.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blocking: Instead of making save_output an argument here, it might be better to compose it from the outside by taking the bytes, which will give the caller more control over the file directory.
blocking q: Can you tell more about how you picked 40? We may want to be very conservative by default (even to only 100% black).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this returns the hash, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh no I had it returning the hash at first but then it created a circular dependency so I removed it but did not update the comment. I will update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blocking q: Hmm, why convert to grayscale first? Won't think convert some full colors to black?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I revised this and updated to check each individual value of the r g b pixel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why .img? Should we use the same format that was passed in?