default value for filter_training_data_by_account

beancount / smart_importer

Augment Beancount importers with machine learning functionality.

MIT License

246 stars 28 forks source link

default value for filter_training_data_by_account #30

Closed johannesjh closed 6 years ago

johannesjh commented 6 years ago

The decorator could try to read importer.file_account() from the decorated importer instance and use this as default value for filter_training_data_by_account.

This would allow the decorator to be used without any arguments (training data can be retrieved from the existing_entries argument to the importer's extract function). The lack of a default value for filter_training_data_byaccount is currently preventing the `test<...>_decoration_with_empty_arguments` unit tests from passing; the unit tests are therefore currently skipped:

PredictPostingsDecorationTest#test_class_decoration_with_empty_arguments
PredictPostingsDecorationTest#test_method_decoration_with_empty_arguments
PredictPostingsDecorationTest#test_class_decoration_without_arguments
PredictPostingsDecorationTest#test_method_decoration_without_arguments

Also, to make the API easier to understand we could rename filter_training_data_by_account to known_account.

zacchiro commented 6 years ago

Having recently picked up smart_importer, I can confirm that filter_training_data_by_account was kind of an obscure name :-) known_account sounds better, but I'm still not 100% sure about the semantics. Is that meant to be the account appearing in the only leg of transactions that bean-extract will synthesize? I haven't passed it as an argument, but smart_importer seems to work fine nonetheless.

zacchiro commented 6 years ago

After reading the code for this, I think this parameter should just be called account, as most importers seems to receive such a named parameter for related purposes. But yeah, supporting using importer.file_account() will most likely make the need of passing an explicit parameter moot.

tarioch commented 6 years ago

I think a default of using file_account probably makes sense. In my setup I have two different cases

importer for a single account, set filter_training_data_by_account with this account (here the file_account) makes sense
importer for multiple accounts, don't set anything, the model uses the existing accounts for the predictions so generally is very good with getting the right values (here I don't want to use file_account as there is no single account)

johannesjh commented 6 years ago

implemented in pull request https://github.com/beancount/smart_importer/pull/40

redstreet commented 1 year ago

The changes made here aren't working well for the use case in #122. Suggestions welcome and appreciated on that ticket. Thank you in advance!