beanmachine
beanmachine copied to clipboard
facebookresearch
Support LBFGS in VI
Summary: Support's torch.optim.LBFGS by passing a closure
Differential Revision: D38171088
Hi @devrimcavusoglu, thank you for your PR! I've glanced through your code and it looks good from what I can see, but it looks like you have a lot of changes that are due to running black (or something similar) on all the files. Can you please remove those changes from this PR so that we can review the code for the new augmentations more easily? @jbitton & I are aware we need to make some formatting changes for black but we will do that in a separate PR. Thanks!
Hi @devrimcavusoglu, thank you for your PR! I've glanced through your code and it looks good from what I can see, but it looks like you have a lot of changes that are due to running
black(or something similar) on all the files. Can you please remove those changes from this PR so that we can review the code for the new augmentations more easily? @jbitton & I are aware we need to make some formatting changes forblackbut we will do that in a separate PR. Thanks!
will do ASAP
@zpapakipos I reverted the black formatting, you can review the changes. I may open another PR perhaps at the end of this week or so for formatting related issues. Btw, test_python is failing due to a brew install, see related issue #115.
Thank you for the review @zpapakipos, I made the according changes to your comments, though I did not change the line-length (black formatting) as the project needs a solid configuration for code style/formatting purposes. For that, I want to address this in a different PR where I will add both configuration files and format the whole codebase according to the rules defined in contributing doc. Thus, my suggestion is to skip code formatting here, and address this on the follow up PR, wdyt ? @jbitton @zpapakipos
@devrimcavusoglu To follow up on your comment above:
For that, I want to address this in a different PR where I will add both configuration files and format the whole codebase according to the rules defined in contributing doc.
That sounds great, happy to review that if you do it!
Thus, my suggestion is to skip code formatting here, and address this on the follow up PR
I see your reasoning, however @jbitton & I would prefer if you commit clean code, at least to the extent that the lines are <90 cols & tabs are the correct number of spaces, etc. We want to keep the style in the codebase consistent, and we know there are some consistent issues when it comes to black, which we appreciate you fixing in a separate PR, but we don't want to introduce more formatting issues than necessary in this PR. Thanks for understanding!
@zpapakipos thank you for the review. I've committed the requested changes. :+1:
@zpapakipos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@zpapakipos Alright, the latest changes are in. I made a few corrections, as well.
Also, I want to make sure you saw this part of my comment above:
One other thing to be aware of: I'll be landing #120 soon, which adds support for bounding boxes to all image augmentations. Once that is landed all new image augmentations will also need to define a function in augly/image/utils/bboxes.py which defines how a bounding box is transformed by that augmentation. Will you be able to help write that function, i.e. do you have a sense of how to compute how an arbitrary pixel in the image's position is changed by these 2 augmentations?
I plan to land the bounding box PR (https://github.com/facebookresearch/AugLy/pull/120) on Monday, and then we should rebase this on top & add a function which computes how the bounding box is transformed. I can help with that if needed, but the hard part is that we need to know how to compute how specific coordinates (i.e. the corners of a bounding box) are transformed by these augmentations.
@zpapakipos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Also, I want to make sure you saw this part of my comment above:
One other thing to be aware of: I'll be landing #120 soon, which adds support for bounding boxes to all image augmentations. Once that is landed all new image augmentations will also need to define a function in augly/image/utils/bboxes.py which defines how a bounding box is transformed by that augmentation. Will you be able to help write that function, i.e. do you have a sense of how to compute how an arbitrary pixel in the image's position is changed by these 2 augmentations?
I plan to land the bounding box PR (#120) on Monday, and then we should rebase this on top & add a function which computes how the bounding box is transformed. I can help with that if needed, but the hard part is that we need to know how to compute how specific coordinates (i.e. the corners of a bounding box) are transformed by these augmentations.
Thank you @zpapakipos, I'll take a look at #120, but I guess it will be a bit tricky to compute how the bbox transformed with distortions. Tbh, I don't come up with an instant practical solution right now for bboxes.
@jbitton Thank you for review, I updated the PR with the latest changes.
Hi @devrimcavusoglu, I spent a few hours working on a function distort_bboxes_helper() to transform the bounding boxes. I have some code which works on some examples, but unfortunately there are quite a few cases where it fails. For example, when the bounding box is partially out of view after the distortion, then the polynomial (the Rdst one you included in the funciton docstrings) that we have to find the roots of has only imaginary roots, and thus we can't compute where the point is transformed to because it is transformed to somewhere out of view.
I just wanted to update you so you know where we are at with this. If you have time to take a look & try to get it working on the cases where it's failing, here is my code & test cases which you can copy & use as a starting point: https://colab.research.google.com/drive/12bpBi8J_rCc7bShmUndlVjcSGuNdgPy3?usp=sharing
Hi @devrimcavusoglu, I have a proposal for how we can pretty quickly merge these changes without you having to wait for me to solve the bounding box problem:
- Please rebase this diff onto the latest version of the main branch. You'll have to do some merging, e.g.
image/utils.pywhich you added to is now calledimage/utils/utils.py, so make sure your changes end up there. - Then create 2 functions here in
image/utils/bboxes.py:
distort_barrel_bboxes_helper(bbox: Tuple, **kwargs) -> Tuple:
raise NotImplementedError("Bounding box support has not yet been added to this augmentation")
and
distort_pincushion_bboxes_helper(bbox: Tuple, **kwargs) -> Tuple:
raise NotImplementedError("Bounding box support has not yet been added to this augmentation")
- In the transform unit tests for these 2 augmentations, add a
try/exceptloop around the tests, like this:
def test_DistortBarrel(self):
try:
self.evaluate_class(imaugs.DistortBarrel(a=0.1), fname="distort_barrel")
except NotImplementedError:
pass
else:
self.assertTrue(
False,
"This augmentation doesn't have bounding box support, so should fail with NotImplementedError"
)
and
def test_DistortPincushion(self):
try:
self.evaluate_class(imaugs.DistortPincushion(a=0.1), fname="distort_pincushion")
except NotImplementedError:
pass
else:
self.assertTrue(
False,
"This augmentation doesn't have bounding box support, so should fail with NotImplementedError"
)
- And then run the image tests & make sure they pass.
I will work on a PR that adds support for transforming bboxes for these 2 augmentations which I'll merge later, but I don't want that to hold up this PR getting resolved. This way users will be able to use these new augmentations, but they will just receive an error if they try to pass in any bounding boxes to alert them that bboxes are not yet supported for these augs. Please make those changes & update the PR if that sounds good, thanks!
Hi @zpapakipos, past weekend I did pulled the last changes and rebased already, but I didnt pushed the changes due to the bbox update you are working on. Having said that, I will push the changes as you requested asap.
I have play around the notebook you linked, I haven't come up with an immediate solution for that problematic cases. However, I think about one "possible" solution for pincushion, but I do not know if it will work on any cases. Perhaps, you develop the idea from here and improve this approach, and even you may apply it for barrel as well. First of all, when I saw the failed pincushion case, naturally I validated my idea to see if the bbox location (you can think of a center precisely) affects how the pincushion fails. So long story short, there is a pattern for pincushion cases that in what direction the bbox center is from the center of canvas (image) the failing part of pincushion is the same direction (you can try several locations center left, top left, bottom right, etc for bbox and see the resulting pincushion effect). Now, from here, I suggest that we draw the resulting bbox from the current boxes coordinates by expanding the bbox in the same direction as the bbox from center of the image. In other words, let's say the bbox is top left to the image center, then we expanding the resulting bbox only towards top left until hitting the image bounds. It'd be better if you inspect visually to better understand the situation. I believe, this pattern does not apply for barrel distortion, but you can observe a different pattern for that. The last thing I want to say is, it is required to validate this assumption for any case which would be harder.
That's great, looking forward to seeing your pushed rebased changes @devrimcavusoglu! Remember to add the bboxes & bbox_format args to the functional & transforms definitions to align with the other augmentations.
Once you've published those changes we should be able to land your changes, and then I'll work on a follow-up diff for the bboxes. I'm thinking of doing an easy but slow solution for this & any other hard augmentations that come up -- basically just create an image with the same width & height as the src image, all black except white inside the bbox, run the augmentation on this image, and then get the transformed bbox by computing the min & max white pixels in each dimension in the augmented image. This should be fairly foolproof for any augmentation (that doesn't affect color) but just take longer than the other solutions we've found for other augmentations like rotate since we have to re-run the augmentation for each bbox.
Hi @zpapakipos, I've updated with the latest changes, I take a look overall, I guess we're good to go. After your review, let me know if any change further is required.
Once you've published those changes we should be able to land your changes, and then I'll work on a follow-up diff for the bboxes. I'm thinking of doing an easy but slow solution for this & any other hard augmentations that come up -- basically just create an image with the same width & height as the src image, all black except white inside the bbox, run the augmentation on this image, and then get the transformed bbox by computing the min & max white pixels in each dimension in the augmented image. This should be fairly foolproof for any augmentation (that doesn't affect color) but just take longer than the other solutions we've found for other augmentations like
rotatesince we have to re-run the augmentation for each bbox.
Great. That seems more solid solution which would work for any augmentation. After all, what I suggested only holds for pincushion distortion.
@zpapakipos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Hi @devrimcavusoglu, update for you: I imported this PR & tried to get the augmentations working in our internal version of the augly codebase (which is linked to this one, so any change here has to be imported & vice versa). But unfortunately ImageMagick is proving to be a really tricky dependency to add. And as @jbitton mentioned before, we are generally hesitant to add new dependencies to our library so we can keep it relatively lightweight & easy to install (and adding wand as a dependency would add a manual step to the installation process since ImageMagick isn't installable via pypi).
Sorry to ask this, but can you please reimplement the distort() function not using wand (or any other new dependencies)? You can use the dependencies we already have like PIL & numpy. I did a quick search of barrel distortion python implementations and for example this one uses cv2, which we also don't want to have to add as a dependency. If you can, please find a way to implement these augmentations without adding a dependency, and then I'll be happy to land it without further delay. Thank you!
Hi @devrimcavusoglu, update for you: I imported this PR & tried to get the augmentations working in our internal version of the augly codebase (which is linked to this one, so any change here has to be imported & vice versa). But unfortunately ImageMagick is proving to be a really tricky dependency to add. And as @jbitton mentioned before, we are generally hesitant to add new dependencies to our library so we can keep it relatively lightweight & easy to install (and adding wand as a dependency would add a manual step to the installation process since ImageMagick isn't installable via pypi).
Sorry to ask this, but can you please reimplement the
distort()function not using wand (or any other new dependencies)? You can use the dependencies we already have like PIL & numpy. I did a quick search of barrel distortion python implementations and for example this one uses cv2, which we also don't want to have to add as a dependency. If you can, please find a way to implement these augmentations without adding a dependency, and then I'll be happy to land it without further delay. Thank you!
Well, I see. It is unfortunate that we cannot use imagemagick because it has many distortion methods not only Barrel and Pincushion. The package you referred to uses only implements subset of distortions in their distort method. On the other hand, ImageMagick implements at least 15 different distortion methods. This is a very large drawback not using ImageMagick for potential upcoming distortion implementations to AugLy. However, all in all if it won't be a good fit for AugLy, we may do a research for a new lighter package or implement it without using any additional packages.
@zpapakipos @jbitton Unfortunately, at this point I have to ask for you to pick up from here and whether you can continue the work on this PR as I will be dealing with a new upcoming package and need to work on a major rework for another package I and my team maintain. If I will take it back from here it will be delayed too long. I'd really appreciate if you can work on the distortion implementations.