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

Contributing Dietz method #27

Closed
francocalvo opened this issue May 29, 2024 · 10 comments
Closed

Contributing Dietz method #27

francocalvo opened this issue May 29, 2024 · 10 comments

Comments

@francocalvo
Copy link
Contributor

Hey!
I've been playing with the code for a while.

First, I packaged it with Poetry and Nix. It should be easy to release it to PyPi.
Secondly, using most of the logic from the current code, I changed the compute_irr for compute_dietz. There are some changes around the code on how the truncate works, but otherwise it's pretty much unchanged.

Would you be interested in a PR for any of those things? It would be cool to have both the time and money weighted performance. It would also allow to add benchmarking.

@blais
Copy link
Member

blais commented May 30, 2024

Sure, sounds interesting.

@francocalvo
Copy link
Contributor Author

francocalvo commented May 30, 2024

Perfect!
This is what it would look like more or less:

image

Do you have a test beancount file to use?

Also, I've done these three things. Please let me know if you'd merge them:

  1. Created a pyproject.toml using Poetry for dependency management, building and tool configuration (pyright, ruff).
  2. Created a 'nix flake'. It would be useful to package it easily to add it to pkg managers like apt.
  3. Fixed all important alerts for those tools above, and fixed some type hints that weren't correct. This is just for the reports.py file. I didn't change any actual logic (except for the demo above, but I can revert that).

The last point comes with many changes to the structure of the file, like replacing os calls with pathlib for example.

@blais
Copy link
Member

blais commented May 30, 2024

I don't have a test Beancount to use. We should probably generate one programmatically.

  1. SGTM
  2. SGTM (if you'll do it, I have zero time for packaging)
  3. SGTM

@francocalvo
Copy link
Contributor Author

Excellent. I'll try to get the PR ready today.

Regarding the usage of Poetry, I'd like to ping @andreasgerstmayr first. It would overwrite your pyproject.toml. Is this OK for you too? I know you use it in a couple of plugins

@andreasgerstmayr
Copy link
Contributor

Regarding the usage of Poetry, I'd like to ping @andreasgerstmayr first. It would overwrite your pyproject.toml. Is this OK for you too? I know you use it in a couple of plugins

Sure, as long as it's still a valid Python package which can be defined as a dependency of other projects :)

@dnicolodi
Copy link

Everything sounds good to me, except the conversion to Poetry. I don't like that it is not designed to compose with the rest of the Python packaging ecosystem, and I don't have time to learn a new way of doing things. What does Poetry bring to the project that cannot be implemented with other more standard tools?

@francocalvo
Copy link
Contributor Author

francocalvo commented May 30, 2024

@andreasgerstmayr good to hear that. It should behave exactly like other packages.

@dnicolodi how is it not designed to compose with the rest of the Python packaging system? You can still pip install it or use it as a dependency in a setup.cfg.

For example, you should be able to install it like:

pip install git+https://github.com/francocalvo/beangrow.git@modified-dietz-returns

PS:
What it brings, In my eyes, is faster builds, a lock file to ensure reproducibility, it respects PEP 518 and PEP 621 better than having a separate setup file (imo). It also creates venv using the lock file.

PPS:
I meant to share this one the last comment about the PEP621: Poetry PR 9135.

@dnicolodi
Copy link

If installing the package is the only thing you want to do, you can do it without modifying the current pyproject.toml. Poetry is also a dependency manager and a virtual environment manager, that requires to use a non-standard definition of the package in pyproject.toml. I don't see why you need to force your use of development tools on everyone.

@francocalvo
Copy link
Contributor Author

francocalvo commented May 30, 2024

Sorry, I edited the comment almost at the same time you commented.
It's not just for installing the package. The point of that was that it shouldn't affect the user.

I'm not trying to force anything on anyone, I'm doing a PR and asked about this specifically. I'm open to change it, and I have it isolated in a commit so I could revert it.

Poetry is the tool I use at work and in personal projects. It has been a very valuable tool for me in deploying Python packages.

@francocalvo
Copy link
Contributor Author

Hey there! Just wanted to share a bit. I intend to work a bit more on this, and publish it if possible. I just changed jobs so life got busy. I'll get to it soon tho. Cheers :)

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

No branches or pull requests

4 participants