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

pix2pix #365

Merged
merged 12 commits into from
Apr 23, 2020
Merged

pix2pix #365

merged 12 commits into from
Apr 23, 2020

Conversation

s1ddok
Copy link
Contributor

@s1ddok s1ddok commented Mar 8, 2020

U-Net based generator model with Facades dataset support. Implementation is a bit rough, but I would love to adjust it in order to be merged into the repo, would love to hear a feedback!

@BradLarson
Copy link
Contributor

That's great, thanks for working on this. As a first step, would you be able to add license headers to all files (see the other models for examples on how this is done) and format these according to the Google Style Guide? For the latter, we use swift-format and have our formatting style codified in the .swift-format file here.

Beyond that, we'd probably want to integrate this into the project's main Package.swift, rather than have it exist as a separate sub-package. At some point, we might even be able to generalize the U-Net layers into something that can be used across examples, but I think this will be fine for now.

I have some smaller comments that I can post inline with the code, but those are the larger formatting issues that come to mind.

@s1ddok
Copy link
Contributor Author

s1ddok commented Mar 9, 2020

Thanks for feedback, I will add the headers and comply with the swift-format

@dabrahams
Copy link
Contributor

@s1ddok gentle ping?

@s1ddok
Copy link
Contributor Author

s1ddok commented Mar 25, 2020

@dabrahams I’m somewhat purposefully stalling here, I want to get CycleGAN PR to be merged first (that one appeared to be a more mature implementation) and then reuse some code between the two.

@s1ddok s1ddok mentioned this pull request Mar 27, 2020
@s1ddok
Copy link
Contributor Author

s1ddok commented Apr 15, 2020

@BradLarson @dabrahams please take a look at updates. I've tried to combine comments from both this and #400. I think the implementations are very close. Please let me know if I can do anything else in order for this to be merged.

@s1ddok
Copy link
Contributor Author

s1ddok commented Apr 20, 2020

@BradLarson @dabrahams sorry for bumping this too often, just don't want this to get outdated too much, I'm all yours for fixes while I'm still in context of this stuff, so please take a look if you have a moment, thanks!

pix2pix/Utils.swift Outdated Show resolved Hide resolved
CycleGAN/main.swift Outdated Show resolved Hide resolved
@s1ddok
Copy link
Contributor Author

s1ddok commented Apr 22, 2020

@BradLarson fixed!

pix2pix/README.md Outdated Show resolved Hide resolved
pix2pix/README.md Outdated Show resolved Hide resolved
pix2pix/README.md Outdated Show resolved Hide resolved
pix2pix/README.md Outdated Show resolved Hide resolved
pix2pix/README.md Outdated Show resolved Hide resolved
Co-Authored-By: Brad Larson <[email protected]>
@s1ddok
Copy link
Contributor Author

s1ddok commented Apr 22, 2020

@BradLarson Sorry I completely forgot about outdated README, I've accepted all your grammar fixes and removed all of TensorBoard mentions.

Copy link
Contributor

@BradLarson BradLarson left a comment

Choose a reason for hiding this comment

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

Thanks for being patient with the suggested changes, and for putting in the hard work to make this go. The results look great, and a general U-Net implementation will be useful for many applications. Really appreciate the work on this.

@BradLarson BradLarson merged commit ae88789 into tensorflow:master Apr 23, 2020
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.

5 participants