refactor: Extract `Duplicate Code` into Helper Function in `ivy\func_wrapper.py` #26572

Sai-Suraj-27 · 2023-10-04T10:23:17Z

PR Description

I identified duplicate sections of code in the following functions:
https://github.com/unifyai/ivy/blob/2bfb19a2ca3b277e95cba987353aa349ef28015e/ivy/func_wrapper.py#L94
https://github.com/unifyai/ivy/blob/2bfb19a2ca3b277e95cba987353aa349ef28015e/ivy/func_wrapper.py#L137

So, I extracted these into their own method/function to increase the readability and decrease the amount of duplicate/repeating code in the file. This refactoring simplifies the code, and reduces redundancy.

Related Issue

Close #26573

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

https://twitter.com/Sai_Suraj_27

…plicate_code

github-actions

PR Compliance Checks Passed!

github-actions · 2023-10-04T10:51:26Z

Thank you for this PR, here is the CI results:

This pull request does not result in any additional test failures. Congratulations!

vedpatwardhan

The changes look good to me, but I'll request @RickSanchezStoic's review just in case this breaks the casting modes. Thanks @Sai-Suraj-27 😄

RickSanchezStoic · 2023-10-06T06:02:34Z

ivy/func_wrapper.py

+        return downcast_helper("complex", dtype, "complex", intersect)
+
+
+def downcast_helper(arg0, dtype, arg2, intersect):


let's change this to arg1

@RickSanchezStoic sir, I made the requested changes. Please check now, Thank you.

RickSanchezStoic · 2023-10-06T06:02:48Z

ivy/func_wrapper.py

+
+
+def downcast_helper(arg0, dtype, arg2, intersect):
+    index = casting_modes_dict[arg0]().index(dtype) - 1


…plicate_code

Sai-Suraj-27 · 2023-10-13T09:11:22Z

@RickSanchezStoic sir, its been a week since I made the requested changes. Please look into the PR when you are free, Thank you.

vedpatwardhan · 2023-10-16T05:48:15Z

Hey @Sai-Suraj-27, can we refactor this further to have a single helper which receives a flag about whether to use the upcast or downcast logic? Seems like the >=0 and <len(casting_modes_dict[arg0]()) condition in the while loop and the index +=1 and index -= 1 are the only 2 things different in the 2 helpers. What do you think? Thanks 😄

…plicate_code

Sai-Suraj-27 · 2023-10-16T16:21:39Z

@vedpatwardhan sir, After observing carefully I see that only in the following case of if "uint" in str(dtype) we are using "int" instead of "uint" again. In all other cases of both downcaster and upcaster functions, we are using the same dtype for the whole condition. So, my doubt is, should it be actually "uint" instead of "int" on line 143? or is it "int" only? I think it should be "uint" but I'm not sure.

https://github.com/unifyai/ivy/blob/19cb1a48f27cf4b3ed27b07fb81b7d7a10d52fba/ivy/func_wrapper.py#L139
https://github.com/unifyai/ivy/blob/19cb1a48f27cf4b3ed27b07fb81b7d7a10d52fba/ivy/func_wrapper.py#L143
Regarding the further refactoring into a single helper function, Sure. I will immediately do it after I get clarity on this, Thank you.

vedpatwardhan · 2023-10-17T04:04:03Z

I think that's just a mistake, can't really think of a reason why it would be int instead of uint on
https://github.com/unifyai/ivy/blob/19cb1a48f27cf4b3ed27b07fb81b7d7a10d52fba/ivy/func_wrapper.py#L143
Good catch @Sai-Suraj-27 😄

…plicate_code

Sai-Suraj-27 · 2023-10-18T11:54:48Z

@vedpatwardhan Sorry for the late response, I have refactored further to use only a single helper function. Please check now.

vedpatwardhan · 2023-10-19T11:59:46Z

ivy/func_wrapper.py

+    index = casting_modes_dict[arg]().index(dtype) + step
+    result = ""
+    if is_upcast:
+        while index < len(casting_modes_dict[arg]()):


ig we could replace this with a single loop instead of 2 where the condition would change based on the value of is_upcast. What do you think?

I thought of the same but I wasn't sure if index will be always >= 0. If the index value is always >= 0, then we can remove the if-else condition and merge the 2 loops to the following:

result = "" while 0 <= index < len(casting_modes_dict[arg]()): if casting_modes_dict[arg]()[index] not in intersect: result = casting_modes_dict[arg]()[index] break index += step return result

Please tell me your view, Thank you.

I think it's safe to assume that index will always be >=0 😄

I made the changes. Can you please look into it now, Thank you.

…plicate_code

vedpatwardhan

lgtm! Feel free to merge the PR @Dsantra92, thanks @Sai-Suraj-27 😄

Sai-Suraj-27 · 2023-11-29T13:06:40Z

Hey, @vedpatwardhan You approved this PR almost a month back but no one merged it. Can you please merge this, thank you.

vedpatwardhan · 2023-11-30T05:45:42Z

Yep merging the PR now, thanks @Sai-Suraj-27 😄

Sai-Suraj-27 added 2 commits October 4, 2023 15:39

Extracted duplicate codes into a function.

bf7da33

Merge branch 'main' of https://github.com/unifyai/ivy into extract_du…

4664126

…plicate_code

Sai-Suraj-27 requested a review from vedpatwardhan as a code owner October 4, 2023 10:23

github-actions bot reviewed Oct 4, 2023

View reviewed changes

ivy-seed assigned umairjavaid Oct 4, 2023

vedpatwardhan reviewed Oct 5, 2023

View reviewed changes

vedpatwardhan requested a review from RickSanchezStoic October 5, 2023 06:23

RickSanchezStoic reviewed Oct 6, 2023

View reviewed changes

Sai-Suraj-27 added 2 commits October 6, 2023 13:42

Merge branch 'main' of https://github.com/unifyai/ivy into extract_du…

ddcb324

…plicate_code

Change argument name from arg0 -> arg1.

a3e9ac4

ivy-seed assigned Dsantra92 and unassigned umairjavaid Oct 14, 2023

Merge branch 'main' of https://github.com/unifyai/ivy into extract_du…

c9574ec

…plicate_code

Sai-Suraj-27 added 4 commits October 18, 2023 15:19

Merge branch 'main' of https://github.com/unifyai/ivy into extract_du…

fe60134

…plicate_code

Merge branch 'main' of https://github.com/unifyai/ivy into extract_du…

5f3ae7c

…plicate_code

Refactored the code further to have only a single helper function.

8cd3aae

Renamed index_step to step for better clariy.

f44f287

vedpatwardhan reviewed Oct 19, 2023

View reviewed changes

Sai-Suraj-27 added 2 commits October 19, 2023 18:54

Merge branch 'main' of https://github.com/unifyai/ivy into extract_du…

eedb32b

…plicate_code

Further refactored to use single while loop.

adcbdb3

Sai-Suraj-27 requested a review from vedpatwardhan October 23, 2023 04:57

vedpatwardhan approved these changes Oct 23, 2023

View reviewed changes

vedpatwardhan merged commit 99f184d into ivy-llc:main Nov 30, 2023
261 of 269 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Extract `Duplicate Code` into Helper Function in `ivy\func_wrapper.py` #26572

refactor: Extract `Duplicate Code` into Helper Function in `ivy\func_wrapper.py` #26572

Sai-Suraj-27 commented Oct 4, 2023 •

edited

Loading

github-actions bot left a comment •

edited

Loading

github-actions bot commented Oct 4, 2023 •

edited

Loading

vedpatwardhan left a comment

RickSanchezStoic Oct 6, 2023

Sai-Suraj-27 Oct 6, 2023

RickSanchezStoic Oct 6, 2023

Sai-Suraj-27 Oct 6, 2023

Sai-Suraj-27 commented Oct 13, 2023

vedpatwardhan commented Oct 16, 2023

Sai-Suraj-27 commented Oct 16, 2023

vedpatwardhan commented Oct 17, 2023

Sai-Suraj-27 commented Oct 18, 2023

vedpatwardhan Oct 19, 2023

Sai-Suraj-27 Oct 19, 2023 •

edited

Loading

vedpatwardhan Oct 19, 2023

Sai-Suraj-27 Oct 19, 2023

vedpatwardhan left a comment

Sai-Suraj-27 commented Nov 29, 2023

vedpatwardhan commented Nov 30, 2023

		return downcast_helper("complex", dtype, "complex", intersect)


		def downcast_helper(arg0, dtype, arg2, intersect):



		def downcast_helper(arg0, dtype, arg2, intersect):
		index = casting_modes_dict[arg0]().index(dtype) - 1

refactor: Extract Duplicate Code into Helper Function in ivy\func_wrapper.py #26572

refactor: Extract Duplicate Code into Helper Function in ivy\func_wrapper.py #26572

Conversation

Sai-Suraj-27 commented Oct 4, 2023 • edited Loading

PR Description

Related Issue

Checklist

Socials

github-actions bot left a comment • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Oct 4, 2023 • edited Loading

vedpatwardhan left a comment

Choose a reason for hiding this comment

RickSanchezStoic Oct 6, 2023

Choose a reason for hiding this comment

Sai-Suraj-27 Oct 6, 2023

Choose a reason for hiding this comment

RickSanchezStoic Oct 6, 2023

Choose a reason for hiding this comment

Sai-Suraj-27 Oct 6, 2023

Choose a reason for hiding this comment

Sai-Suraj-27 commented Oct 13, 2023

vedpatwardhan commented Oct 16, 2023

Sai-Suraj-27 commented Oct 16, 2023

vedpatwardhan commented Oct 17, 2023

Sai-Suraj-27 commented Oct 18, 2023

vedpatwardhan Oct 19, 2023

Choose a reason for hiding this comment

Sai-Suraj-27 Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

vedpatwardhan Oct 19, 2023

Choose a reason for hiding this comment

Sai-Suraj-27 Oct 19, 2023

Choose a reason for hiding this comment

vedpatwardhan left a comment

Choose a reason for hiding this comment

Sai-Suraj-27 commented Nov 29, 2023

vedpatwardhan commented Nov 30, 2023

refactor: Extract `Duplicate Code` into Helper Function in `ivy\func_wrapper.py` #26572

refactor: Extract `Duplicate Code` into Helper Function in `ivy\func_wrapper.py` #26572

Sai-Suraj-27 commented Oct 4, 2023 •

edited

Loading

github-actions bot left a comment •

edited

Loading

github-actions bot commented Oct 4, 2023 •

edited

Loading

Sai-Suraj-27 Oct 19, 2023 •

edited

Loading