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

Adding Type Hints for Enhanced Code Clarity and Maintenance #922

Merged
merged 3 commits into from Dec 18, 2023

Conversation

RazaProdigy
Copy link
Contributor

Objective:
This pull request introduces type hints to the existing codebase for deepface.py. The addition of type hints aims to enhance code clarity, facilitate maintenance, and improve the development experience for contributors and users of the repository.

Key Changes:

  • Added type hints to function parameters and return types across various modules.
  • Ensured compatibility with existing code by performing thorough checks and adhering to the current logic and structure.
  • Enhanced readability and understandability of the code, especially in complex functions.

No Breaking Changes: These additions are non-intrusive and backward compatible. They do not alter the logic or functionality of the existing code.

Request for Review:
I request the maintainers to review these changes. Feedback and suggestions for further refinement are highly appreciated.

align=True,
normalization="base",
):
img1_path: str,
Copy link
Owner

Choose a reason for hiding this comment

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

all image path variables (img_path, img1_path, img2_path) can be string or numpy array. So, these should accept numpy array as

from typing import Union
import numpy as np

def verify(img1_path: Union[str, np.ndarray]):
   pass

@@ -45,7 +46,7 @@
# -----------------------------------


def build_model(model_name):
def build_model(model_name: str) -> Any:
Copy link
Owner

Choose a reason for hiding this comment

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

build_model returns one of these objects:

  • keras.engine.functional.Functional
  • deepface.basemodels.DlibResNet.DlibResNet
  • deepface.basemodels.SFace.SFaceModel

So, this should be typed as

from typing import Union
from keras.engine.functional import Functional
from deepface.basemodels import DlibResNet, SFace

def build_model(model_name: str) -> Union[Functional, DlibResNet.DlibResNet, SFace.SFaceModel]:
    pass

We should then get rid of Any import in this module.

Copy link
Owner

Choose a reason for hiding this comment

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

Or you may consider to use Functional and any. Because except 2 models, this function returns Functional.

Union[Functional, Any]

@RazaProdigy
Copy link
Contributor Author

I've updated the type hints in the image paths and build_model function as per your suggestions. The new commit is pushed to this PR. Could you please take another look when you have a chance?

Thank you!

align=True,
silent=False,
):
img_path: Union[str, np.ndarray],,
Copy link
Owner

Choose a reason for hiding this comment

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

Double comma at the end of the line breaks the code

@serengil serengil merged commit 8a5f287 into serengil:master Dec 18, 2023
@serengil serengil mentioned this pull request Dec 20, 2023
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.

None yet

2 participants