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

feat(predictor): Add support for denylisting accounts #131

Merged
merged 3 commits into from
Sep 15, 2024

Conversation

hlieberman
Copy link
Contributor

There are many occasions when the training data set may include accounts which should not be predicted (e.g., accounts used for manual reconciliation of AR/AP). This feature allows the user to stop the predictor from learning these accounts, thus preventing contamination of the training set, without having to maintain a separate filtered copy of their transactions.

NB: Currently, all CI runs are broken because of beancount 3.0 hitting PyPi. Manual testing with beancount<3.0.0 shows all tests passing.

@hlieberman hlieberman changed the title feat(predictor): Add support for blacklisting accounts feat(predictor): Add support for denylisting accounts Aug 12, 2024
@tarioch
Copy link
Collaborator

tarioch commented Aug 14, 2024

Hi @hlieberman thank you very much for your contribution, I fixed it that for now smart-importer forces beancount < 3 and added the ci job on the PRs. Would you mind rebasing your PR so that the ci checks can run?

There are many occasions when the training data set may include accounts which
should not be predicted (e.g., accounts used for manual reconciliation of
AR/AP). This feature allows the user to stop the predictor from learning these
accounts, thus preventing contamination of the training set, without having to
maintain a separate filtered copy of their transactions.

NB: Currently, all CI runs are broken because of beancount 3.0 hitting PyPi.
Manual testing with beancount<3.0.0 shows all tests passing.
@hlieberman hlieberman reopened this Aug 14, 2024
@hlieberman
Copy link
Contributor Author

Not sure why Github decided that a force push should close this PR there, but... reopened and rebased. :)

Copy link
Member

@yagebu yagebu left a comment

Choose a reason for hiding this comment

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

I added documentation for this parameter and fixed the formatting

@yagebu yagebu merged commit 1b12989 into beancount:main Sep 15, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants