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

Draft: Merging TemplateFunction into Function #831

Closed
wants to merge 81 commits into from

Conversation

ansharlubis
Copy link
Collaborator

No description provided.

@certik
Copy link
Contributor

certik commented Aug 3, 2022

@ansharlubis looks like we got to the point with @Oshanath that we need this merged, so that we can start using it.

@ansharlubis what is your time line to get this PR ready? It looks like the only thing left is to uncomment and fix the integration tests. Do you need help with this?

@ansharlubis
Copy link
Collaborator Author

ansharlubis commented Aug 4, 2022

@ansharlubis looks like we got to the point with @Oshanath that we need this merged, so that we can start using it.

@ansharlubis what is your time line to get this PR ready? It looks like the only thing left is to uncomment and fix the integration tests. Do you need help with this?

I can uncomment it right away, but it will cause the integration tests to fail.

@certik
Copy link
Contributor

certik commented Aug 4, 2022

Thanks.

So this is now our highest priority. We need to get these tests to pass.

@ansharlubis any idea why they fail?

@Oshanath and @ansharlubis: try to take this PR, reproduce the failure of the test on your machine, and let's try to figure out how to fix it. Let me know what you find. Try to do your best today. Then we have our meeting tomorrow, so we can work on it together as well.

@ansharlubis
Copy link
Collaborator Author

ansharlubis commented Aug 4, 2022

I think I found out why, although I still don't know how to solve it yet. It seems that the addition of type_params into Function in ASR causes the array translation into LLVM to mess up.

@czgdp1807
Copy link
Collaborator

czgdp1807 commented Aug 4, 2022

It seems that the addition of type_params into Function in ASR causes the array translation into LLVM to mess up.

Well, make_Function_t doesn't receive elemental attribute correctly in your branch. You are always passing false in that attribute (see below)

https://github.com/ansharlubis/lpython/blob/a489c1bf12212cbe6646b311cc5c3e6dd7eed801/src/lpython/semantics/python_ast_to_asr.cpp#L2366

It should be, s_access, deftype, vectorize, bindc_name); instead of s_access, deftype, false, bindc_name);

See the code from main branch for the same thing,

s_access, deftype, vectorize, bindc_name);

@czgdp1807
Copy link
Collaborator

@certik @ansharlubis @Oshanath The tests are now passing. Feel free to clean up the git history before merging.

@ansharlubis
Copy link
Collaborator Author

@czgdp1807 thank you very much!

tests/tests.toml Outdated
Comment on lines 159 to 161
# [[test]]
# filename = "../integration_tests/test_numpy_04.py"
# asr = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Another one that needs to be uncommented:

Suggested change
# [[test]]
# filename = "../integration_tests/test_numpy_04.py"
# asr = true
[[test]]
filename = "../integration_tests/test_numpy_04.py"
asr = true

@certik
Copy link
Contributor

certik commented Aug 4, 2022

Do not rebase this PR. Let's only keep pushing things in. That way we can get back to the version that @czgdp1807 fixed (thanks!).

@ansharlubis / @Oshanath can you please enable that one other test (see my comment above). Then merge the latest main into this branch and resolve conflicts? Get all tests passing again.

After that, I want to review more. I will be out for a few hours, but in the evening I'll give it a final review. Then we polish the patches and merge.

@certik
Copy link
Contributor

certik commented Aug 4, 2022

You have to resolve these conflicts by merging the latest main branch:

Screen Shot 2022-08-04 at 4 39 03 PM

@certik
Copy link
Contributor

certik commented Aug 5, 2022

You have to run ./run_tests.py -u to update the tests.

@ansharlubis
Copy link
Collaborator Author

Got it, I just updated it with a wrong build. I'll have to fix it again.

@certik
Copy link
Contributor

certik commented Aug 5, 2022

This was merged in #900.

@certik certik closed this Aug 5, 2022
@ansharlubis ansharlubis deleted the remove-template branch August 5, 2022 18:14
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.

3 participants