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: Replaced lambda expressions with def functions #27738

Merged
merged 4 commits into from
Dec 25, 2023

Conversation

Sai-Suraj-27
Copy link
Contributor

@Sai-Suraj-27 Sai-Suraj-27 commented Dec 12, 2023

PR Description

I see that, at few places in the codebase, a lambda expression is being used instead of a def. I found all the places where this is happening using the ruff tool, and replaced all of those lambda expressions with a def function.

Related Issue

Closes #

Checklist

  • Did you add a function?
  • Did you add the tests?
  • Did you run your tests and are your tests passing?
  • Did pre-commit not fail on any check?
  • Did you follow the steps we provided?

Socials

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

PR Compliance Checks

Thank you for your Pull Request! We have run several checks on this pull request in order to make sure it's suitable for merging into this project. The results are listed in the following section.

Issue Reference

In order to be considered for merging, the pull request description must refer to a specific issue number. This is described in our contributing guide and our PR template.
This check is looking for a phrase similar to: "Fixes #XYZ" or "Resolves #XYZ" where XYZ is the issue number that this PR is meant to address.

@ivy-leaves ivy-leaves added JAX Frontend Developing the JAX Frontend, checklist triggered by commenting add_frontend_checklist NumPy Frontend Developing the NumPy Frontend, checklist triggered by commenting add_frontend_checklist Ivy Functional API Ivy API Experimental Run CI for testing API experimental/New feature or request labels Dec 12, 2023
Copy link
Contributor

@KareemMAX KareemMAX left a comment

Choose a reason for hiding this comment

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

Seems like there are some lint failures that are happening, can you fix those?

Copy link
Contributor

@vedpatwardhan vedpatwardhan left a comment

Choose a reason for hiding this comment

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

I'm not fully sure if we should be replacing all lambda functions by def (espectially the ones that are simple like lambda x: isinstance(x, bool)), what do you think @KareemMAX? Thanks for creating the PR @Sai-Suraj-27 😄

@Sai-Suraj-27
Copy link
Contributor Author

Sai-Suraj-27 commented Dec 13, 2023

Seems like there are some lint failures that are happening, can you fix those?

@KareemMAX, The lint is failing because the docformatter and pydocstyle linters are conflicting about closing of Multi-line docstring quotes should be on a separate line or on the same line.
Edit: These are fixed with this PR (#27761)

@KareemMAX
Copy link
Contributor

I'm not fully sure if we should be replacing all lambda functions by def (espectially the ones that are simple like lambda x: isinstance(x, bool)), what do you think @KareemMAX? Thanks for creating the PR @Sai-Suraj-27 😄

I don't think it will hurt, the functions will have equivalent AST after all. Unless you prefer lambdas from a readability/writability point of view

@KareemMAX KareemMAX merged commit 580db06 into ivy-llc:main Dec 25, 2023
8 of 141 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ivy API Experimental Run CI for testing API experimental/New feature or request Ivy Functional API JAX Frontend Developing the JAX Frontend, checklist triggered by commenting add_frontend_checklist NumPy Frontend Developing the NumPy Frontend, checklist triggered by commenting add_frontend_checklist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants