feature and fix: added support for casting modes to give the original… #23439

RickSanchezStoic · 2023-09-11T18:15:13Z

added support for casting modes to give the original expected output type, also modified promote_types to accept None

… expected output dtype, also modified promote_types to accept None

github-actions · 2023-09-11T18:16:16Z

Thanks for contributing to Ivy! 😊👏
Here are some of the important points from our Contributing Guidelines 📝:
1. Feel free to ignore the run_tests (1), run_tests (2), … jobs, and only look at the display_test_results job. 👀 It contains the following two sections:
- Combined Test Results: This shows the results of all the ivy tests that ran on the PR. ✔️
- New Failures Introduced: This lists the tests that are passing on main, but fail on the PR Fork. Please try to make sure that there are no such tests. 💪
2. The lint / Check formatting / check-formatting tests check for the formatting of your code. 📜 If it fails, please check the exact error message in the logs and fix the same. ⚠️🔧
3. Finally, the test-docstrings / run-docstring-tests check for the changes made in docstrings of the functions. This may be skipped, as well. 📚
Happy coding! 🎉👨‍💻

vedpatwardhan · 2023-09-12T04:25:11Z

ivy/func_wrapper.py

@@ -1162,9 +1162,13 @@ def _wrap_function(
    return to_wrap


-def casting_modes_ops(fn):
+def casting_modes_ops(fn, dtype_determinator=None):


everything else looks great! let's probably rename the argument to ret_dtype_target or something which might be more informative, what do you think?
I'll request @MahmoudAshraf97's review as well on this PR, thanks for making the changes @RickSanchezStoic 💪

MahmoudAshraf97 · 2023-09-12T11:11:23Z

ivy/func_wrapper.py

+        if not to_cast and ret_dtype_target:
+            for arg in ret_dtype_target:
+                to_cast = ivy.promote_types(
+                    to_cast,
+                    ivy.dtype(
+                        args[arg_names.index(arg)] if arg not in kwargs else kwargs[arg]
+                    ),
+                )


this does not take into consideration when the inputs are python scalars or other objects that do not have a dtype attribute, we should stick to using promote_types_of_inputs to guarantee consistent results, also to prevent the existence of two identical promotion codes which will double the maintenance efforts

another problem is that this doesn't take into consideration default values that are optional but still affect the output dtype

promote_types_of_inputs will directly modify the inputs, which shouldn't be done here. The args that are being read here are the ones explicitly passed to the decorator and are supposed to have a type attribute. Lastly, we don't have any promotion rules for scalars( or do we have that?) and if they really do affect the output dtype then we have a problem. Can you post an example that does that?

Hey @MahmoudAshraf97 I remember having this conversation a couple of days back but don't fully remember the specifics, could you please share an example where this would be applicable? Thanks and apologies 😅

We do have promotion rules for scalars included in ivy.promote_types_of_inputs but not in the promotion table hence they are promoted to the default dtype in ivy.promote_types which will lead to an inconsistent promotion results

ivy.promote_types(float_scalar, float16) = float32
ivy.promote_types_of_inputs(float_scalar, float16) = float16

Actually ivy.promote_types should always receive dtypes (ivy.Dtype or ivy.NativeDtype), so ideally the result from ivy.promote_types_of_inputs is the one which would be used anyway. So ideally float16 would be the promoted dtype as a result 😄

MahmoudAshraf97 · 2023-09-12T12:01:50Z

I think we will need a method to test if this decorator is working as expected or not, my suggestion is to enable testing functions using their unsupported dtypes, this will guarentee consistency with frameworks that do support these dtypes

vedpatwardhan

Just a minor change, rest looks good to me. I agree with @MahmoudAshraf97, would be great to have tests for these too. Also, does this cover all the other corner cases we were discussing? Thanks 😄

ivy/func_wrapper.py

MahmoudAshraf97 · 2023-09-14T21:39:46Z

Hey @RickSanchezStoic , I guess you reverted my commit when you committed, can you please revise?

vedpatwardhan

Just requested a few changes! Let's focus on the functions which use ivy.promote_types_of_inputs for now for a version 0 as there isn't really an exhaustive way to test if there are issues in the output dtype for the other functions until we integrate casting modes into the tests (cc @sherry30), I'll also request @MahmoudAshraf97's review on this PR. Thanks @RickSanchezStoic 😄

vedpatwardhan · 2023-10-04T05:44:14Z

ivy/func_wrapper.py

+                    if arg not in kwargs:
+                        args[arg_names.index(arg)] = (
+                            arg_mod
+                            if not ivy.isarray(args[arg_names.index(arg)])


this should be ivy.is_array instead of ivy.isarray

vedpatwardhan · 2023-10-04T05:44:16Z

ivy/func_wrapper.py

+                    else:
+                        kwargs[arg] = (
+                            arg_mod
+                            if not ivy.isarray(args[arg_names.index(arg)])


vedpatwardhan · 2023-10-04T05:44:17Z

ivy/func_wrapper.py

@@ -1300,7 +1336,8 @@ def _wrapped(func):
            if "frontends" in func.__module__:
                # it's a frontend func, no casting modes for this
                return func
-            return casting_modes_ops(func)
+
+            return casting_modes_ops(func, ret_dtype_target)


let's remove the blank line above this and pass ret_dtype_target as a keyword argument

ivy/functional/ivy/data_type.py

github-actions · 2023-10-06T06:25:11Z

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

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

feature and fix: added support for casting modes to give the original…

05eb9cd

… expected output dtype, also modified promote_types to accept None

RickSanchezStoic requested a review from vedpatwardhan as a code owner September 11, 2023 18:15

ivy-leaves added the Ivy Functional API label Sep 11, 2023

ivy-seed assigned vedpatwardhan Sep 11, 2023

vedpatwardhan suggested changes Sep 12, 2023

View reviewed changes

vedpatwardhan requested a review from MahmoudAshraf97 September 12, 2023 06:28

minor fixes

4a7ea79

MahmoudAshraf97 suggested changes Sep 12, 2023

View reviewed changes

adress issues in review comments

7f66fd9

RickSanchezStoic requested a review from vedpatwardhan September 13, 2023 00:55

vedpatwardhan suggested changes Sep 13, 2023

View reviewed changes

ivy/func_wrapper.py Outdated Show resolved Hide resolved

RickSanchezStoic added 2 commits September 13, 2023 15:30

Merge branch 'unifyai:main' into casting_modes

9d56326

resolution for None

fa285a2

AwkNinja mentioned this pull request Sep 22, 2023

feat(frontends): Implement rfft in PaddlePaddle frontend and fix fft for Tensorflow backend #19454

Merged

RickSanchezStoic added 2 commits October 3, 2023 14:57

Merge branch 'main' into casting_modes

3f10783

added fix for cross cast and promote types of inputs for non array args

d339b31

RickSanchezStoic requested a review from vedpatwardhan October 3, 2023 10:46

vedpatwardhan suggested changes Oct 4, 2023

View reviewed changes

Merge branch 'unifyai:main' into casting_modes

5b0173d

changes requested

f4fd104

RickSanchezStoic requested a review from KareemMAX as a code owner October 6, 2023 06:30

RickSanchezStoic added 2 commits October 6, 2023 08:12

demos update

e12360b

Merge remote-tracking branch 'upstream/main' into casting_modes

23b2c4b

RickSanchezStoic closed this Oct 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature and fix: added support for casting modes to give the original… #23439

feature and fix: added support for casting modes to give the original… #23439

RickSanchezStoic commented Sep 11, 2023

github-actions bot commented Sep 11, 2023

vedpatwardhan Sep 12, 2023

MahmoudAshraf97 Sep 12, 2023

MahmoudAshraf97 Sep 12, 2023

RickSanchezStoic Sep 12, 2023

vedpatwardhan Sep 13, 2023

MahmoudAshraf97 Oct 5, 2023

vedpatwardhan Oct 6, 2023

MahmoudAshraf97 commented Sep 12, 2023

vedpatwardhan left a comment

MahmoudAshraf97 commented Sep 14, 2023

vedpatwardhan left a comment

vedpatwardhan Oct 4, 2023

vedpatwardhan Oct 4, 2023

vedpatwardhan Oct 4, 2023

github-actions bot commented Oct 6, 2023

feature and fix: added support for casting modes to give the original… #23439

feature and fix: added support for casting modes to give the original… #23439

Conversation

RickSanchezStoic commented Sep 11, 2023

github-actions bot commented Sep 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MahmoudAshraf97 commented Sep 12, 2023

vedpatwardhan left a comment

Choose a reason for hiding this comment

MahmoudAshraf97 commented Sep 14, 2023

vedpatwardhan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 6, 2023