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

Fmean, mean overload functions implemented #877

Merged
merged 5 commits into from Aug 9, 2022

Conversation

Madhav2310
Copy link
Contributor

@Madhav2310 Madhav2310 commented Aug 3, 2022

No description provided.

@certik
Copy link
Contributor

certik commented Aug 3, 2022

Currently this test is not enabled in llvm:

RUN(NAME test_statistics     LABELS cpython)

Can you isolate all bugs from this that LPython still does not support and report them? Let's get them fixed.

I would recommend to only implement things that actually fully compile with LPython. Otherwise we'll end up with a huge library that does not work.

Comment on lines 230 to 233
for i in range(k):
for j in range(i+1,k):
if x[i]>x[j]:
x[i],x[j]=x[j],x[i]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This code is repeated many times. How about creating a new function, say _sort and reuse it?

Copy link
Contributor Author

@Madhav2310 Madhav2310 Aug 4, 2022

Choose a reason for hiding this comment

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

I made the following function

def _sort(x: list[i32]):
    k: i32 = len(x)
    i: i32
    j: i32
    for i in range(k):
        for j in range(i+1,k):
            if x[i]>x[j]:
                x[i],x[j]=x[j],x[i]

and passed the list by reference in the following manner:

_sort({x})

However, when I try to overload the sort function for f64, it throws an error. Any suggestions for alternative workarounds? @Smit-create

@czgdp1807
Copy link
Collaborator

Lists are supported now in LLVM. So please enable LLVM for your test.

@Madhav2310
Copy link
Contributor Author

Lists are supported now in LLVM. So please enable LLVM for your test.

I'm getting the following error when enabling LLVM for this:

[ 85%] Generating test_statistics.o
Internal Compiler Error: Unhandled exception
Traceback (most recent call last):
  Binary file "/home/madhav/lpython/src/bin/lpython", in _start()
  Binary file "/lib/x86_64-linux-gnu/libc.so.6", in __libc_start_main()
  File "/home/madhav/lpython/src/bin/lpython.cpp", line 867, in ??
    return compile_python_to_object_file(arg_file, outfile, runtime_library_dir, lpython_pass_manager, compiler_options, time_report);
  File "/home/madhav/lpython/src/bin/lpython.cpp", line 350, in ??
    compiler_options.disable_main, compiler_options.symtab_only, infile);
  File "/home/madhav/lpython/src/lpython/semantics/python_ast_to_asr.cpp", line 4166, in LFortran::LPython::python_ast_to_asr(Allocator&, LFortran::LPython::AST::ast_t&, LFortran::diag::Diagnostics&, bool, bool, bool, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >)
    LFORTRAN_ASSERT(asr_verify(*tu));
  File "/home/madhav/lpython/src/libasr/asr_verify.cpp", line 541, in LFortran::asr_verify(LFortran::ASR::TranslationUnit_t const&, bool)
    v.visit_TranslationUnit(unit);
  File "/home/madhav/lpython/src/libasr/asr_verify.cpp", line 111, in LFortran::ASR::VerifyVisitor::visit_TranslationUnit(LFortran::ASR::TranslationUnit_t const&)
    this->visit_symbol(*a.second);
  File "/home/madhav/lpython/src/libasr/../libasr/asr.h", line 3694, in LFortran::ASR::BaseVisitor<LFortran::ASR::VerifyVisitor>::visit_symbol(LFortran::ASR::symbol_t const&)
    void visit_symbol(const symbol_t &b) { visit_symbol_t(b, self()); }
  File "/home/madhav/lpython/src/libasr/../libasr/asr.h", line 3480, in ??
    case symbolType::Module: { v.visit_Module((const Module_t &)x); return; }
  File "/home/madhav/lpython/src/libasr/asr_verify.cpp", line 217, in LFortran::ASR::VerifyVisitor::visit_Module(LFortran::ASR::Module_t const&)
    this->visit_symbol(*a.second);
  File "/home/madhav/lpython/src/libasr/../libasr/asr.h", line 3694, in LFortran::ASR::BaseVisitor<LFortran::ASR::VerifyVisitor>::visit_symbol(LFortran::ASR::symbol_t const&)
    void visit_symbol(const symbol_t &b) { visit_symbol_t(b, self()); }
  File "/home/madhav/lpython/src/libasr/../libasr/asr.h", line 3482, in ??
    case symbolType::Function: { v.visit_Function((const Function_t &)x); return; }
  File "/home/madhav/lpython/src/libasr/asr_verify.cpp", line 280, in LFortran::ASR::VerifyVisitor::visit_Function(LFortran::ASR::Function_t const&)
    visit_stmt(*x.m_body[i]);
  File "/home/madhav/lpython/src/libasr/../libasr/asr.h", line 3708, in LFortran::ASR::BaseVisitor<LFortran::ASR::VerifyVisitor>::visit_stmt(LFortran::ASR::stmt_t const&)
    void visit_stmt(const stmt_t &b) { visit_stmt_t(b, self()); }
  File "/home/madhav/lpython/src/libasr/../libasr/asr.h", line 3513, in ??
    case stmtType::If: { v.visit_If((const If_t &)x); return; }
  File "/home/madhav/lpython/src/libasr/../libasr/asr.h", line 4006, in LFortran::ASR::BaseWalkVisitor<LFortran::ASR::VerifyVisitor>::visit_If(LFortran::ASR::If_t const&)
    self().visit_expr(*x.m_test);
  File "/home/madhav/lpython/src/libasr/../libasr/asr.h", line 3750, in LFortran::ASR::BaseVisitor<LFortran::ASR::VerifyVisitor>::visit_expr(LFortran::ASR::expr_t const&)
    void visit_expr(const expr_t &b) { visit_expr_t(b, self()); }
  File "/home/madhav/lpython/src/libasr/../libasr/asr.h", line 3557, in ??
    case exprType::IntegerCompare: { v.visit_IntegerCompare((const IntegerCompare_t &)x); return; }
  File "/home/madhav/lpython/src/libasr/../libasr/asr.h", line 4317, in LFortran::ASR::BaseWalkVisitor<LFortran::ASR::VerifyVisitor>::visit_IntegerCompare(LFortran::ASR::IntegerCompare_t const&)
    self().visit_expr(*x.m_left);
  File "/home/madhav/lpython/src/libasr/../libasr/asr.h", line 3750, in LFortran::ASR::BaseVisitor<LFortran::ASR::VerifyVisitor>::visit_expr(LFortran::ASR::expr_t const&)
    void visit_expr(const expr_t &b) { visit_expr_t(b, self()); }
  File "/home/madhav/lpython/src/libasr/../libasr/asr.h", line 3550, in ??
    case exprType::FunctionCall: { v.visit_FunctionCall((const FunctionCall_t &)x); return; }
  File "/home/madhav/lpython/src/libasr/asr_verify.cpp", line 498, in LFortran::ASR::VerifyVisitor::visit_FunctionCall(LFortran::ASR::FunctionCall_t const&)
    require(symtab_in_scope(current_symtab, x.m_name),
  File "/home/madhav/lpython/src/libasr/asr_verify.cpp", line 60, in LFortran::ASR::VerifyVisitor::require(bool, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, LFortran::Location const&)
    require(cond, msg);
LCompilersException: ASR verify failed: 2822:2824: FunctionCall::m_name `_mod@__lpython_overloaded_0___mod` cannot point outside of its symbol table
make[2]: *** [CMakeFiles/test_statistics.dir/build.make:74: test_statistics.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:2012: CMakeFiles/test_statistics.dir/all] Error 2
make: *** [Makefile:101: all] Error 2

I tried to understand why the symbol table it throwing an error but couldn't figure it out. Suggestions please? @czgdp1807

@czgdp1807
Copy link
Collaborator

The error is not related lists. It is due to incorrect symbol being used in the FunctionCall to mod. This needs some investigation. @Smit-create Can you please try looking into it?

@Smit-create
Copy link
Collaborator

Yeah, will look into it.

@Smit-create Smit-create changed the title Fmean, Median, Median_low, median_high, mean overload functions implemented Fmean, mean overload functions implemented Aug 8, 2022
@Smit-create
Copy link
Collaborator

I have removed the median functions because the diff was getting larger and had some issues to fix. Rest all is working fine.

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.

That's excellent that such list usage already works.

Thanks!

@czgdp1807
Copy link
Collaborator

Great. Thanks @Smit-create for working on this and completing this.

@Smit-create Smit-create merged commit 4a4d406 into lcompilers:main Aug 9, 2022
@Madhav2310
Copy link
Contributor Author

I have removed the median functions because the diff was getting larger and had some issues to fix. Rest all is working fine.

@Smit-create could you please explain what you mean by this? That way I'll be careful of the issue in question as I implement further functions

@Madhav2310
Copy link
Contributor Author

I have removed the median functions because the diff was getting larger and had some issues to fix. Rest all is working fine.

@Smit-create could you please explain what you mean by this? That way I'll be careful of the issue in question as I implement further functions

How could I improve on the median function? I also have another PR open with Variance and stdev functions.


for i in range(k):
sum += x[i]
return sum/k
Copy link
Contributor

Choose a reason for hiding this comment

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

Btw, these are great candidates to try using the new templates. The templates are still experimental, and we still need to implement restrictions, so things will change, but the current state should be able to compile such functions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree. @ansharlubis @Oshanath May be, you can try templates on these functions and reduce the implementation size? What do you folks say?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might be a little bit early, since it will slow @ansharlubis down if we have to do any refactoring, which we will. We can however do it on a branch / Draft PR that is not merged. That would be great, just to see if it works, and to iron out any bugs, but I would not merge into master yet.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It kind of work in an example I added in PR #921, it still uses the hard-coded restrictions though and don't do much type checks.

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.

None yet

6 participants