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/CAK21%2BhOfX-7EzE27ir4nyNuUUk0E-D6_aLKuB0G9X9302tZdMQ%40mail.gmail.com.
