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

Refactor MobileNetBackbone for readability #1806

Closed
jbischof opened this issue May 19, 2023 · 8 comments · Fixed by #1838
Closed

Refactor MobileNetBackbone for readability #1806

jbischof opened this issue May 19, 2023 · 8 comments · Fixed by #1838

Comments

@jbischof
Copy link
Contributor

Our backbone modeling files are very large and hard to read. @LukeWood proposed two changes in #1745:

  1. move aliases to their own file
  2. move helpers below model declarations

#1745 provides a nice template for this change but sat around for a long time and now has big merge conflicts. Replicate this change rebased on the current master for keras_cv/models/backbones/mobilenet_v3/.

@ID6109
Copy link
Contributor

ID6109 commented May 19, 2023

I'll be starting/resuming work on #1637 , #1635 , #1634 , and #1628 next week. I'll implement the improvement suggested in #1745 in the PRs for these anyway, so how about I take up this, #1807 , #1808 , and #1809 as well? If all goes to plan, we should have ready-to-review PRs for all the issues mentioned by the end of the month.

@jbischof
Copy link
Contributor Author

jbischof commented May 26, 2023

I assigned you #1806 but can you also comment on #1807, #1808, and #1809? Otherwise it won't let me. Thanks!

@jbischof
Copy link
Contributor Author

@ID6109 really appreciate your help! Re: backbone conversions let's take one or two at time since it would take too long for a single person to do all of them.

@ID6109
Copy link
Contributor

ID6109 commented May 30, 2023

@jbischof I have #1642 , #1643 , #1649 , #1759 , and #1829 open for these conversions. Should I pause my work for #1635 , #1634 , and #1628 ?

@jbischof
Copy link
Contributor Author

@ID6109 my recommendation would be to take only the number/scope of issues that you can actively work on and make sure the issue is assigned to you. This allows us to keep tabs on what's being worked and close/reassign anything stale. Again, appreciate your work!

@ID6109
Copy link
Contributor

ID6109 commented May 30, 2023

Thanks @jbischof! I'm comfortable with simultaneously working on my open PRs as of now. Most of these issues have been assigned to me, and I am taking up the rest because they have gone stale. I've earlier commented for #1649 (on #1630) and #1829 (on #1637). I'll also comment on #1635 and #1628 so they're formally assigned to me.

@jbischof
Copy link
Contributor Author

@ID6109 I think the right strategy is to get assignments for what you can work on in the next week or so and then pick up more if you have time. This is a TON of work 😅

@ID6109
Copy link
Contributor

ID6109 commented May 30, 2023

I have PRs open for all the issues assigned to me. I'll focus on the comments I get on these for now rather than going for more. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants