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

Various functions #139

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

igheorghita
Copy link
Contributor

@igheorghita igheorghita commented Nov 15, 2021

  • tests added / passed
  • passes tox

Added implementations for ADDRESS, COUNTA, COUNTBLANK, COLUMNS, ROWS, FILTER, and a few financial functions (IRR, PMT, PPMT). The financial functions are computed using the numpy-financial package and I didn't investigate how the IRR implementation in Excel might differ from it (Excel has a guess parameter, but numpy-financial doesn't use a guess). Perhaps there's a reason that pycel isn't already using this package - let me know if that's the case.

I think the implementations for COLUMNS and ROWS could be improved perhaps using the excel_helper decorator, but I wasn't sure how to properly apply that I think.

FILTER is also a bit of a strange case since it's only for Microsoft 365. I added a test xlsx file for this one.

Happy to change any of the implementations or tests.

Edit: Do you know why flake8 is failing on src/pycel/lib/lookup.py and tests/lib/test_financial.py? I looks like that error message is a bug and it might happen when there are linter errors in the file, but when I run flake8 locally, I don't see anything...

Edit part 2: Fixed the linter warnings being hidden by the flake8 bug.

@jiboncom
Copy link

jiboncom commented Jun 30, 2022

Thank you for doing this! One question thou. I just started with pycel, but shouldn't you add the financial library to the default modules for this to work?

I tried installing from your branch with pip, and I couldn't get pycel you evaluate a cell containing the irr() command or even recognize the financial functions. So I compared the results of searching for "financial" against the results of "engineering" and your lib file is missing in one file. I manually modified that file and deleted the pycache folders and it worked.

excelformula.py #Line:525

    default_modules = (
        'pycel.excellib',
        'pycel.lib.date_time',
        'pycel.lib.engineering',
        'pycel.lib.information',
        'pycel.lib.logical',
        'pycel.lib.lookup',
        'pycel.lib.stats',
        'pycel.lib.text',
        'math'

Anyway, many thanks! I was stuck with this for some time.

@igheorghita
Copy link
Contributor Author

Ah thanks for catching that. You're probably right! I can update the PR soon, but I'm not sure when (if ever) it'll get merged... haha

@dgorissen
Copy link
Owner

Hi Both. Thanks for putting this together. Sorry for the silence here, this is not a project I develop actively anymore. But if both of you confirm the changes work I can merge.

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

3 participants