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

Fix spin-offs calculations #524

Merged
merged 10 commits into from
Jun 18, 2024
Merged

Fix spin-offs calculations #524

merged 10 commits into from
Jun 18, 2024

Conversation

nihn
Copy link
Contributor

@nihn nihn commented Jun 2, 2024

This diff fixes Spin-Off cost calculations (both for source and dest shares). Details of the calculations can be found in the comment of the main method. As we do not have a good DB for spin-offs the "source" share has to be provided by user during the run, the adjustments to the prices are then explained in generated .pdf file.

Additionally:

  • Adding few missing transaction types to Schwab parser.
  • Better error handling here and there.
  • Move transactions (disposals, acquisitions, portfolios) to the instance variables to avoid passing them around all the time and making the class easier to understand (IMHO).

I'll add tests after initial review round.

@KapJI KapJI added feature New feature or request bug Something isn't working labels Jun 3, 2024
Copy link
Owner

@KapJI KapJI left a comment

Choose a reason for hiding this comment

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

Thanks for implementing this! It needs some test to make sure the calculations are correct.

cgt_calc/main.py Show resolved Hide resolved
cgt_calc/main.py Outdated
)
available_quantity = min(disposal_quantity, same_day_quantity)
if has_key(self.acquisition_list, date_index, symbol):
sd_acquisition = self.acquisition_list[date_index][symbol]
Copy link
Owner

Choose a reason for hiding this comment

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

I'm ok with bnb for bed_and_breakfast but this one hurts readability IMO.

Suggested change
sd_acquisition = self.acquisition_list[date_index][symbol]
same_day_acquisition = self.acquisition_list[date_index][symbol]

cgt_calc/main.py Outdated
)
assert bnb_acquisition.quantity <= acquisition.quantity

sd_disposal = (
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
sd_disposal = (
same_day_disposal = (

cgt_calc/main.py Outdated
disposal = self.disposal_list[date_index][symbol]
disposal_quantity = disposal.quantity
proceeds_amount = disposal.amount
disposal_fees = disposal.fees
Copy link
Owner

Choose a reason for hiding this comment

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

Only used once and not modified, can be used as disposal.fees

cgt_calc/main.py Outdated
@@ -409,37 +509,30 @@ def process_disposal(
fees=fees,
new_quantity=current_quantity,
new_pool_cost=current_amount,
spin_off=None,
Copy link
Owner

Choose a reason for hiding this comment

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

It's default to None.

Suggested change
spin_off=None,

cgt_calc/main.py Outdated
Comment on lines 219 to 222
ticker = input(
"For a spin off, please enter the original ticker from which the new "
"stock was spinned off: "
)
Copy link
Owner

Choose a reason for hiding this comment

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

How user will understand what split this is about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point :D

cgt_calc/main.py Outdated Show resolved Hide resolved
cgt_calc/main.py Outdated
while True:
# This would ideally be fetched from some stock DB but yfinance does not
# provide any info on SpinOffs
ticker = input(
Copy link
Owner

Choose a reason for hiding this comment

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

Can you add option to provide required info from text file? To make it possible to rerun script without specifying any input. Ideally, prices from yfinance can be stored there as well. E.g. here after fetching you can store data in some cache file and check its presence.

cgt_calc/main.py Outdated
Comment on lines 227 to 235
total_amount = sum(
t.amount
for transactions in self.acquisition_list.values()
if (t := transactions.get(ticker))
) - sum(
t.amount
for transactions in self.disposal_list.values()
if (t := transactions.get(ticker))
)
Copy link
Owner

Choose a reason for hiding this comment

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

Why not use current value from portfolio?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, this is left over from a previous, invalid version of this diff

cgt_calc/main.py Outdated
for transactions in self.disposal_list.values()
if (t := transactions.get(ticker))
)
share_of_original_cost = total_amount / (amount + total_amount)
Copy link
Owner

Choose a reason for hiding this comment

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

Why total amount increased here (amount + total_amount)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Assume the market value of MMM shares is £90 per share and SOLV shares
is £10 per share immediately after the spin-off.
The total market value is £90 + £10 = £100.

we need to consider cost basis of "spin-off pool" as value of original stock and new stock.

@KapJI KapJI removed the feature New feature or request label Jun 4, 2024
nihn added 2 commits June 9, 2024 08:49
* Fix calculations for case where there were multiple buy and sell transactions before spin-off - make spin-off a CalculationEntry
* Adjust pdf formating
* Add test for spin-offs.
@KapJI
Copy link
Owner

KapJI commented Jun 17, 2024

Can you rebase on the latest revision?

Btw, will it work correct for B&B case like in #511?

@nihn
Copy link
Contributor Author

nihn commented Jun 17, 2024

I don't think it will change anything for that B&B case as I am not touching stock splits but B&B should be handled correctly in case of spin-offs (AFAICT, as it just modifies the pool price, I am not tax advisor though :)).

@nihn
Copy link
Contributor Author

nihn commented Jun 17, 2024

Any idea why the signals are failing? I pulled the branch and it works on my machine:
image

@vmartinv
Copy link
Collaborator

Any idea why the signals are failing? I pulled the branch and it works on my machine

You're not including the spin_off_handler file in the commit.

@nihn
Copy link
Contributor Author

nihn commented Jun 18, 2024

You're not including the spin_off_handler file in the commit.

How did it work before is beyond my understanding though, thanks! :D

@KapJI
Copy link
Owner

KapJI commented Jun 18, 2024

Thanks for working on this! 🚀

@KapJI KapJI merged commit e7355c8 into KapJI:main Jun 18, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants