search

HumanSignal / labelImg

LabelImg is now part of the Label Studio community. The popular image annotation tool created by Tzutalin is no longer actively being developed, but you can check out Label Studio, the open source data labeling tool for images, text, hypertext, audio, video and time-series data.
https://youtu.be/p0nR2YsCY_U
MIT License
22.51k stars 6.27k forks source link

Switch to pep-8 #717

Closed Cerno-b closed 3 years ago

Cerno-b commented 3 years ago

The codebase seems to adhere to the camelcase notation generally used in C++, likely to keep in line with Qt's function naming.

I was wondering whether you might be open to switching to the Python-recommended naming scheme defined in the PEP-8 https://www.python.org/dev/peps/pep-0008/

Of course, PEP-8 clearly states in the beginning that it should not be applied blindly, however this has become a somewhat standard in the python world that most code analysis tools support and it makes for a more unified and well-readable code base.

I would offer to make the conversion to PEP-8 for labelImg if you would go along with it, but I understand that this is a huge change that touches almost all code files and will cause pending pull-requests to be affected, so it might not be high (or at all) on your priority list.

I would probably do this in multiple steps, with the first step being to convert all the variable and function names to snake-case, so that each line of code stays exactly where it is, which would make adapting outstanding pull-requests easy without breaking anything. Later, I would make some style changes that actually improve code readability so that all warnings that a tool like lint would report, vanish. This makes future bugs easier to identify and avoid while coding.

Please let me know if you are open to the idea of switching to PEP-8.

tzutalin commented 3 years ago

Sounds good to me.

Maybe you can leverage a tool to do this conversion

Cerno-b commented 3 years ago

@tzutalin Pretty sure there is a way to do this automatically, otherwise PyCharm has a semi-automatic mode that works fairly well. Should be a few hours of manual work.

Do you have any preference when you would want to do this? Are there any outstanding pull requests that you want to merge beforehand? Merging anything afterwards that has been branched off a pre-pep8-state could be a bit of a pain, since almost every line of code will be touched in the conversion.

Cerno-b commented 3 years ago

@tzutalin Okay, I'm done with the first step. Do you have some sort of checklist for testing whether everything works? I've run the unit tests but it seems like two failed even before my change. I could play around with the tool for a bit to see if everything seems to work fine, but if you have a more robust way of testing, I'd like to double-check

tzutalin commented 3 years ago

We don’t have auto integration tests yet. You need to do manual integrations at the moment.

Basically, you can do these manual tests:

  1. Open the app, load images and label images, and save them. You should be able to save annotation files. Make sure that you can save them for XML , yolo format, and so on.
  2. When you label, make sure you can resize rect and rename them
  3. Make sure hotkeys shown in README can work
  4. Try to check all features in the menu can work. For example, ‘auto save’ can work
Cerno-b commented 3 years ago

@tzutalin It looks fine, except for the the CreateML format which seems to have problems saving and loading. I cross-checked with the master branch before my changes and the bug is there as well. I will open a separate issue for this

I did all the checks you suggested and my changes work without problems, so I'll open a PR for you to review

Cerno-b commented 3 years ago

@tzutalin Currently working on stage two by removing more warnings and making the code a bit prettier. Do you still need the dependency to Qt4 and Python 2? I could just refactor them out while I am at it.

I found a few loose ends that could point towards undesired behaviour, I'll mark them as to discuss when I open the second PR after the first one passes.

tzutalin commented 3 years ago

Hi Cerno, Thanks for doing this. I would rather keep python2 support at the moment. We can refactor step by step. Let’s refactor coding and naming convention first.

Cerno-b commented 3 years ago

@tzutalin Sure, no problem. I'm pretty much done with the changes to stage 2 (need to do some thorough testing though). I have it on a separate branch and will create a PR once the current PR is merged.

Not trying to pressure you or anything, but could you give me some insight on how much time it generally takes you to review and merge a PR?

tzutalin commented 3 years ago

Hi I just saw your pull request. I will check it today

https://github.com/tzutalin/labelImg/pull/720

tzutalin commented 3 years ago

PR looks good to me. Thanks for great work again!

Cerno-b commented 3 years ago

I added a separate issue for stage 2: https://github.com/tzutalin/labelImg/issues/725

It has its own Pull Request: https://github.com/tzutalin/labelImg/pull/723