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

default value for filter_training_data_by_account #30

Closed
johannesjh opened this issue Apr 17, 2018 · 5 comments · Fixed by #40
Closed

default value for filter_training_data_by_account #30

johannesjh opened this issue Apr 17, 2018 · 5 comments · Fixed by #40
Assignees

Comments

@johannesjh
Copy link
Collaborator

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_by_account 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
Copy link

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
Copy link

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
Copy link
Collaborator

tarioch commented Apr 24, 2018

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 added a commit that referenced this issue Apr 30, 2018
…` or sometimes to be be more specific to `known_account`.
@johannesjh johannesjh self-assigned this Apr 30, 2018
@johannesjh
Copy link
Collaborator Author

implemented in pull request #40

@redstreet
Copy link

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants