Thanks for collaborating with me Martin! I have the 'trivial' version working and have tested it on my personal ledger. I added several testcases, but need more before I put up a PR. The current implementation doesn't currently rebook at average on augmentations: that requires tweaking the code in booking_full. The booking methods are /only/ invoked now to disambiguate lots in reductions. I'll have some time this weekend and hope to have a PR ready soon.
On Thu, Nov 26, 2020 at 9:10 PM Martin Blais <[email protected]> wrote: > Reviewed. I think you got to most of the issues (see comments). > Totally fine starting with a constrained version. > The issue of reducing within a transaction is a thorny one, but I think > it'll be fine to just reduce against the inventory that comes before the > transaction. > Thank you, > > > On Tue, Nov 17, 2020 at 12:44 PM Ben Blount <[email protected]> wrote: > >> First draft on the clarification doc is ready for review. >> <https://docs.google.com/document/d/1rclTmxJzq-4Na1UPY0XfS9Yzuv_SuHpdfHtabNsUH0E/edit#> >> In short, I suggest we start with the feature in the most restrictive >> state, where multiple cost currencies, and specifying cost basis manually >> isn't allowed for accounts using the AVERAGE booking method. AVERAGE cost >> booking is still immensely useful even if these cornercases are not >> supported. Users that require them can still use the current workaround of >> NONE booking with manual aggregations. >> >> My biggest open question is around inside-transaction posting ordering, >> and if beancount already has conventions for how to handle that. I also >> need to think through http://furius.ca/beancount/doc/self-reductions a >> bit more to see if that will affect this. >> >> I hit most of the edge cases you mentioned when building my average >> rebooking plugin, but it's quite possible that I'll hit more once I get >> into the testcases/implementation. I'll move those into the doc for >> discussion / posterity as they come up. >> >> On Mon, Nov 16, 2020 at 9:53 PM Martin Blais <[email protected]> wrote: >> >>> On Tue, Nov 17, 2020 at 12:42 AM Martin Blais <[email protected]> wrote: >>> >>>> On Tue, Nov 17, 2020 at 12:21 AM Ben Blount <[email protected]> wrote: >>>> >>>>> Great, glad to hear Martin! >>>>> >>>>> I'll start with a doc similar to your docs that outlines the >>>>> implementation decisions to be made, offering options for each. Once we >>>>> have consensus on those and if it still looks doable for me, I'll start >>>>> building out the testcases in a branch on my own repo. >>>>> >>>> >>>> SGTM. I think a good way is to write out small scenarios in unit tests >>>> illustrating those cases I enumerated and you'll think of more. The >>>> loader.load_doc() helper and the corresponding parse_string() for expected >>>> outputs make it really easy to do that without having to write any real >>>> code. >>>> >>>> >>>> I saw some references to previous testcases written in a 'booking' >>>>> branch. I don't see that branch anymore, perhaps it was on the old git >>>>> provider. Do you know where these are / are they worth reusing? >>>>> >>>> >>>> I... can't remember. It's been too long. I ported the "booking_fixes" >>>> branch, maybe that's the one. >>>> >>> >>> Actually no, there's nothing interesting in that branch. I had started >>> to fix https://github.com/beancount/beancount/issues/167 and >>> https://github.com/beancount/beancount/issues/168 in booking_fixes, >>> unrelated. >>> >>> >>> >>>> >>>> Looking fwd, >>>> >>>> >>>> >>>> >>>> >>>>> On Mon, Nov 16, 2020 at 6:48 PM Martin Blais <[email protected]> wrote: >>>>> >>>>>> On Mon, Nov 16, 2020 at 9:02 PM Ben Blount <[email protected]> wrote: >>>>>> >>>>>>> First off, thank you so much for such a great tool! >>>>>>> One challenge I haven't been able to solve yet with beancount is >>>>>>> tracking basis for my US 401k - average cost booking. >>>>>>> >>>>>>> I did quite a bit of digging into the code, TODOs, and the mailing >>>>>>> list. I actually implemented average cost booking on augmentation and >>>>>>> reduction as a plugin, but it's not possible with just a plugin due to >>>>>>> booking running before plugins do (makes sense- having resolved lots is >>>>>>> very useful). >>>>>>> >>>>>>> My summary from investigations: >>>>>>> >>>>>>> - It's >>>>>>> <https://groups.google.com/g/beancount/c/SP5KeksHbCk/m/aIMfFDMRBQAJ> >>>>>>> been >>>>>>> <https://groups.google.com/g/beancount/c/72PN-d-je40/m/lHQZf6O0CQAJ> >>>>>>> asked >>>>>>> <https://groups.google.com/g/beancount/c/dOrCPTbHZKY/m/vhPipQaVAQAJ> >>>>>>> a >>>>>>> fair bit on the mailing list. >>>>>>> - The best workaround currently is to use the "NONE" booking >>>>>>> method and manually summarize it everywhere you need to. Martin >>>>>>> has a decent workaround here. >>>>>>> <https://groups.google.com/g/beancount/c/TjOnum255Ps/m/hDFkdqCaAwAJ> >>>>>>> I >>>>>>> wrote a plugin to automate some parts of Martin's >>>>>>> - Since it touches the heart of the balance update mechanism it >>>>>>> could break many things if done without care. Martin previously >>>>>>> (understandably) would rather not accept it as a contribution. >>>>>>> <https://groups.google.com/g/beancount/c/SP5KeksHbCk/m/bOXNBFOjAwAJ> >>>>>>> >>>>>>> Martin: Do you still feel the same way? I am wondering if perhaps >>>>>>> you'd want contributions of unit tests for it? I'd love to do anything I >>>>>>> can to help get AVERAGE booking going, my 401k has a lot of history and >>>>>>> contributions so it's a big chore to track. >>>>>>> >>>>>> >>>>>> Yes, nothing much has changed. It's possible to do, I just think it >>>>>> might create a lot of unexpected work, but it's possible. FWIW, I want >>>>>> that >>>>>> feature too! If you're willing to (a) include tests covering most corner >>>>>> cases for the code you're adding, (b) finish coverage of the feature to >>>>>> the >>>>>> extent of not leaving too many loose ends and handle most of the corner >>>>>> cases (will take more time than one might think IMHO, but what do I know >>>>>> about your level of enthusiasm for this problem?) and (c) support it when >>>>>> people report things that aren't working, at least until it's really >>>>>> stable, I'm happy to review PRs. You can start the work in a branch too >>>>>> if >>>>>> you prefer. >>>>>> >>>>>> There's already a method for it, and you don't even have to change >>>>>> the syntax, start here: >>>>>> >>>>>> https://github.com/beancount/beancount/blob/master/beancount/parser/booking_method.py#L173 >>>>>> >>>>>> The way I think it would best be implemented is that on an >>>>>> augmentation or reduction to an account marked as AVERAGE booking method, >>>>>> merge all the lots from that account in each commodity to a per-commodity >>>>>> lot with the average cost. Implementing that naively is not difficult. >>>>>> Optionally, you could trigger the merging only on reduction (would work >>>>>> too, less rounding errors, but less consistency). But... find a way to >>>>>> handle rounding errors gracefully. Also, the resulting costs will be >>>>>> with >>>>>> long fractions (not rounded)... or will they be rounded? If the costs >>>>>> aren't rounded, are users going to be able to specify a cost value to >>>>>> reduce? How would that match against the number they input if the costs >>>>>> have 20 fractional digits? (maybe won't be that important since there's >>>>>> only a single position to reduce from anyway and one could always elide >>>>>> the >>>>>> cost to match against it? Is that true? I'd like to see it in tests, >>>>>> maybe >>>>>> that works well). How about the case where there are multiple commodities >>>>>> in the same account? Or the same commodity priced in different >>>>>> currencies, >>>>>> what should occur? People will do these things. And how to deal with the >>>>>> case that would result in a negative cost (e.g. inventory = 100 HOOL {60 >>>>>> USD}, + reduction of -11 HOOL {600 USD}, what should happen? Raise an >>>>>> error? Or allow negative cost? Okay, so say you decide to issue an error. >>>>>> What is this occurs in a transient situation, e.g., that state only >>>>>> occurs >>>>>> as a temporary state of an inventory while processing a jumble of >>>>>> reductions and augmentations from the same transaction, the end state of >>>>>> which would be legitimate?). My intuition based on the past booking >>>>>> features I've implemented is that a lot of stuff like that will come up >>>>>> as >>>>>> you implement it, and more that we aren't thinking about yet, but it's >>>>>> definitely worth a shot. You can add tests for all these corner cases. If >>>>>> the issues I raised in this paragraph sound new, I think the right way to >>>>>> go about this is to try it out isolating each of these corner cases with >>>>>> dedicated unit tests for each, and carefully document each of the cases >>>>>> in >>>>>> a shared doc with examples as they come up, and solicit feedback and/or >>>>>> find creative solutions to them, and document them, as liberal comments >>>>>> in >>>>>> the code or in that doc. >>>>>> >>>>>> As for v3, if it works well, I would just convert that to the >>>>>> equivalent C++ code, like the rest of the core (slowly underway). >>>>>> >>>>>> >>>>>> >>>>>> Thanks! >>>>>>> >>>>>>> -- >>>>>>> You received this message because you are subscribed to the Google >>>>>>> Groups "Beancount" group. >>>>>>> To unsubscribe from this group and stop receiving emails from it, >>>>>>> send an email to [email protected]. >>>>>>> To view this discussion on the web visit >>>>>>> https://groups.google.com/d/msgid/beancount/CACGEkZvQ_bFqf%3DKsriEC5qMJOj5r0Oh8c-bF6Cj-hT%3D2eAS7Gg%40mail.gmail.com >>>>>>> <https://groups.google.com/d/msgid/beancount/CACGEkZvQ_bFqf%3DKsriEC5qMJOj5r0Oh8c-bF6Cj-hT%3D2eAS7Gg%40mail.gmail.com?utm_medium=email&utm_source=footer> >>>>>>> . >>>>>>> >>>>>> -- >>>>>> You received this message because you are subscribed to a topic in >>>>>> the Google Groups "Beancount" group. >>>>>> To unsubscribe from this topic, visit >>>>>> https://groups.google.com/d/topic/beancount/1RRUo04hNbI/unsubscribe. >>>>>> To unsubscribe from this group and all its topics, send an email to >>>>>> [email protected]. >>>>>> To view this discussion on the web visit >>>>>> https://groups.google.com/d/msgid/beancount/CAK21%2BhNStTJ7QkSvKWJpYWDTwcUc%3DggROujgPB72_Ge0HaFW6A%40mail.gmail.com >>>>>> <https://groups.google.com/d/msgid/beancount/CAK21%2BhNStTJ7QkSvKWJpYWDTwcUc%3DggROujgPB72_Ge0HaFW6A%40mail.gmail.com?utm_medium=email&utm_source=footer> >>>>>> . >>>>>> >>>>> -- >>>>> You received this message because you are subscribed to the Google >>>>> Groups "Beancount" group. >>>>> To unsubscribe from this group and stop receiving emails from it, send >>>>> an email to [email protected]. >>>>> To view this discussion on the web visit >>>>> https://groups.google.com/d/msgid/beancount/CACGEkZtDSSdAFQKJ-afk_gKmwi%3DSDP2tfHw5zVycsY1G5RKx0g%40mail.gmail.com >>>>> <https://groups.google.com/d/msgid/beancount/CACGEkZtDSSdAFQKJ-afk_gKmwi%3DSDP2tfHw5zVycsY1G5RKx0g%40mail.gmail.com?utm_medium=email&utm_source=footer> >>>>> . >>>>> >>>> -- >>> You received this message because you are subscribed to a topic in the >>> Google Groups "Beancount" group. >>> To unsubscribe from this topic, visit >>> https://groups.google.com/d/topic/beancount/1RRUo04hNbI/unsubscribe. >>> To unsubscribe from this group and all its topics, send an email to >>> [email protected]. >>> To view this discussion on the web visit >>> https://groups.google.com/d/msgid/beancount/CAK21%2BhPyW6r4xyXvdK4ye6nNVsUcLpwd5h6oFCHDSGvXiLFZeQ%40mail.gmail.com >>> <https://groups.google.com/d/msgid/beancount/CAK21%2BhPyW6r4xyXvdK4ye6nNVsUcLpwd5h6oFCHDSGvXiLFZeQ%40mail.gmail.com?utm_medium=email&utm_source=footer> >>> . >>> >> -- You received this message because you are subscribed to the Google Groups "Beancount" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/beancount/CACGEkZs%3DCUNVzh7EK80Ne7prd0MUPRmJ5y%3DuD3PgfJEh7YPN_g%40mail.gmail.com.
