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

[numpy] add trunc #2316

Merged
merged 9 commits into from
Sep 12, 2023
Merged

[numpy] add trunc #2316

merged 9 commits into from
Sep 12, 2023

Conversation

khushi-411
Copy link
Contributor

Hi,

The PR adds the trunc function to the lpython library. It follows the issue #959.

cc: @certik @czgdp1807

Looking forward to hearing your thoughts and feedback on this addition. Thank you for your time!

One thing that's been on my mind is whether we have style checks like flake8, clang, or lintrunner in lpython. Do you think it would be a good idea to include them?

Thanks! :)

@certik
Copy link
Contributor

certik commented Sep 7, 2023

Intrinsic functions should be implemented using the IntrinsicFunction approach. See for example how sin/cos is implemented.

@khushi-411
Copy link
Contributor Author

Intrinsic functions should be implemented using the IntrinsicFunction approach. See for example how sin/cos is implemented.

Hi, @certik! Thanks for looking into it. I've added trunc using the IntrinsicFunction approach. Could you please take another look and let me know if I could improve it somewhere?

@certik
Copy link
Contributor

certik commented Sep 8, 2023

Excellent, this looks great. @Thirumalai-Shaktivel would you mind reviewing this please? Didn't we have some shorter way to define these common intrinsics? I don't see how right now, so this might be the way to do it.

@Thirumalai-Shaktivel
Copy link
Collaborator

I think we can use the create_exp_macro macro. But, we have to update the macro to include the instantiate function as we do for create_trig.

The only difference between them is the type create_trig accepts both real and complex but create_exp_macro functions accept only the real type.

I will add this to my to-do list, to redesign the handling.

@Thirumalai-Shaktivel
Copy link
Collaborator

Thirumalai-Shaktivel commented Sep 8, 2023

I see, we cannot include an instantiate function in the create_exp_macro macro.
I think let's keep the changes as is and merge. Later while redesigning the handling, I will try to fix this.

@certik certik marked this pull request as draft September 8, 2023 17:50
@certik
Copy link
Contributor

certik commented Sep 8, 2023

After you fix #2316 (comment), go ahead and click on "Ready for review", I'll give it one final review.

@certik
Copy link
Contributor

certik commented Sep 8, 2023

@Thirumalai-Shaktivel yes I agree, let's merge this PR as is, and we'll redesign later. We should probably create macros for the most common cases and just use them.

Copy link
Contributor

@certik certik left a comment

Choose a reason for hiding this comment

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

I think this is fine.

@certik certik marked this pull request as ready for review September 11, 2023 09:47
@certik
Copy link
Contributor

certik commented Sep 11, 2023

@czgdp1807 I think you probably want to merge this after the ASR sync is done, correct?

@czgdp1807
Copy link
Collaborator

Correct.

@khushi-411
Copy link
Contributor Author

khushi-411 commented Sep 11, 2023

Thanks very much for reviewing the PR @certik, @Thirumalai-Shaktivel, and @Shaikh-Ubaid!

@certik
Copy link
Contributor

certik commented Sep 11, 2023

Thanks @khushi-411 for the PR! We'll merge it once the ASR sync is complete.

@certik certik merged commit df414fb into lcompilers:main Sep 12, 2023
12 checks passed
@khushi-411 khushi-411 deleted the lp_trunc branch September 13, 2023 03:55
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.

5 participants