Skip to content

Replace Coco to COCO in enum fields #5244

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

Closed
wants to merge 1 commit into from

Conversation

zhiqwang
Copy link
Contributor

This modification is just to follow the suggestion in #5088 (comment) .

cc @datumbox

Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

Thanks @zhiqwang. I'm not sure if @yoshitomo-matsubara was also looking into it renaming.

As you can see below I've noted that we missed a couple of names (except if your intention was indeed to replace only coco as you say in your PR title). At any case, my recommendation would be to do all replacements in a single PR, so that we can review the proposal as a whole (assess readability).

Hopefully the change is very straightforward and done mostly with IDEs (the reason why we opted for Enums instead of strings). If you prefer to split the work with @yoshitomo-matsubara, and do it piece by piece we can also create a feature branch send the individual PRs against it and finally at the end send one single PR for the entire change assessing the proposal as a whole.

We are flexible so let me know your thoughts. Thanks!

Copy link
Contributor Author

@zhiqwang zhiqwang left a comment

Choose a reason for hiding this comment

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

If you prefer to split the work with @yoshitomo-matsubara, and do it piece by piece we can also create a feature branch send the individual PRs against it and finally at the end send one single PR for the entire change assessing the proposal as a whole.

Hi @datumbox . Sure, I can help @yoshitomo-matsubara with this part of the work.

@facebook-github-bot
Copy link

💊 CI failures summary and remediations

As of commit 4f03f8f (more details on the Dr. CI page):


Commit 4f03f8f was recently pushed. Waiting for builds...


This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

@yoshitomo-matsubara
Copy link
Contributor

yoshitomo-matsubara commented Jan 21, 2022

Hi @datumbox
Yes, I am still working on it locally. To avoid conflict, I'd like to leave detection/ to @zhiqwang and will focus on other modules.

Hi @zhiqwang
Is that ok?

Also for the above cases, I think COCO_Legacy -> COCO_LEGACY ImageNet1K_FEATURES -> IMAGENET1K_FEATURES per #5088 (comment)

@datumbox
Copy link
Contributor

@yoshitomo-matsubara Thanks for confirming you are working on it.

To avoid conflict, I'd like to leave detection/ to @zhiqwang and will focus on other modules.

I'm not sure that's possible. The detection area contains references of the classification backbones, so your PRs will end up touching the same files at the same time.

@yoshitomo-matsubara
Copy link
Contributor

@datumbox

I'm not sure that's possible. The detection area contains references of the classification backbones, so your PRs will end up touching the same files at the same time.

Then, should I wait until this PR is merged and then start working on the rest?

@datumbox
Copy link
Contributor

@zhiqwang would it be OK to let @yoshitomo-matsubara send the full PR, given he has already started it? We can definitely get your help on the review. Of course there are alternatives we can do, such as careful merging and resolving conflicts but we might as well do the simplest thing. Let me know if that's OK with you, thanks!

@zhiqwang
Copy link
Contributor Author

zhiqwang commented Jan 22, 2022

Hi @datumbox and @yoshitomo-matsubara ,

would it be OK to let @yoshitomo-matsubara send the full PR, given he has already started it?

Yep, I think it would be better to let @yoshitomo-matsubara go ahead and sent the full PR, as such I'm closing this ticket, and thank you both for kindly reviewing this.

@yoshitomo-matsubara
Copy link
Contributor

Thank you @zhiqwang !

I just made local commits and sent a PR #5257

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

Successfully merging this pull request may close these issues.

4 participants