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

Fuzzy Date plugin #496

Merged
merged 12 commits into from Dec 1, 2017

Conversation

Projects
None yet
2 participants
@tuxlifan
Contributor

tuxlifan commented Oct 27, 2017

Cherry-picked from import_plugin_fuzzy_date_bind branch (#493) to have only the new Fuzzy Date ImportBindPlugin

Checks for a split amount match with transactions from 4 days before until 4 days after.
Base confidence (weight) is set to 0.75 so we can improve with additional description matching, etc.
In case of importing older data one might want to set DELTA_T to a higher value than 4, e.g. 5, 6 or even 7, due to longer banking delays back in the days.

tuxlifan added some commits Oct 9, 2017

New ImportBindPlugin: Fuzzy Date
Checks for a split amount match with transactions from 4 days before until
4 days after.
Base confidence is set to 0.75 so we can improve with additional memo
matching, etc.
@hsoft

This comment has been minimized.

Owner

hsoft commented Oct 27, 2017

The flake8 fail seems to be due to a new version of flake8 adding a new check, which moneyguru fails. I'll fix it soon and add the fix to this PR.

@tuxlifan

This comment has been minimized.

Contributor

tuxlifan commented Oct 27, 2017

I was just about to create an issue for that after checking in a panic how I could have messed up some file in core/gui (and git blame assuring me I didn't) :)

For reference: E741 was introduced by flake8 version 3.5.0 and says "do not use variables named ‘l’, ‘O’, or ‘I’".

FWIW I vote for ignoring E741 since coding should be done with proper fonts anyway and "l" (lower case L) is too good an abbreviation for {line, letter, layout, ...} and you can pry away the 'i' for index from my dead cold fingers (unless they really only mean "I" (upper case I) in which case you will never be able to pry away the 'i', even from my dead cold fingers), but YMMV.

from core.plugin import ImportBindPlugin, EntryMatch
#
# TODO: possible error source: date range selected for moneyguru.

This comment has been minimized.

@hsoft

hsoft Oct 28, 2017

Owner

I can already answer to that: existing entries, yes. For imported entries, it's evident that it's not filtered by the selected date range, but I wasn't 100% sure for the existing_entries part so I looked. We can see at https://github.com/hsoft/moneyguru/blob/master/core/gui/import_window.py#L301 that entries are taken from Account (https://github.com/hsoft/moneyguru/blob/master/core/model/account.py#L74), which is filled by Document's Oven, which is bound to the date range.

Now that I think about it, I don't know why its like that. Initially, when I implemented import matching, I was a bit afraid of transaction matching coming out of nowhere and matching 10 years old transactions, but I also think that binding existing_entries to the date range, which is a visual filter which could very well be set to a very weird range, is a pretty bad idea. We should probably change that.

This comment has been minimized.

@tuxlifan

tuxlifan Nov 22, 2017

Contributor

Thanks for pointing out where to look! I need to further dive into this.
My gut feeling is that we do need some kind of bounds to avoid thousands of old and irrelevant existing_entries being passed to plugins with potentially expensive operations and eventually slowing down everything too much but I don't have the full picture & behavior of mG figured out yet.

BASE_CONFIDENCE = FuzzyDateBind.BASE_CONFIDENCE
PENALTIES = FuzzyDateBind.PENALTIES
logging.basicConfig(level=logging.DEBUG)

This comment has been minimized.

@hsoft

hsoft Oct 28, 2017

Owner

I'm guessing that you wrote that line to see debug messages on test failures? that's a good idea. we should do that for all tests. I would recommend (if it doesn't break tests) that we place this in https://github.com/hsoft/moneyguru/blob/master/core/tests/conftest.py

This comment has been minimized.

@tuxlifan

tuxlifan Nov 22, 2017

Contributor

Correct. Seems to work fine, pushed in 3343fed below.

@mark.parametrize("do_refine_matches", [(False), (True)])
def test_single_imported_typical(do_refine_matches):

This comment has been minimized.

@hsoft

hsoft Oct 28, 2017

Owner

Before I start commenting on your tests, I need to start with the most important: I really like the way you test. It's good stuff.

Now with the question: what do you parametrize here? I understand that we're waiting on FuzzyMemoBind, but even when we're going to get there, do_refine_matches will have no effect on the test themselves. isn't the do_refine_matches argument supposed to affect the resulting matches variable?

This comment has been minimized.

@tuxlifan

tuxlifan Nov 22, 2017

Contributor

Thank you :) My train of thought for the parametrization was to ensure that the additional fuzzy memo logic would not interfere with these minimal examples written for exlusively FuzzyDateBind logic, i.e. to prove that it actually does not have any (unwanted) effect.

The first run (False) would be with only the logic inside FuzzyDateBind for which these tests are written for. The second run (True) would enable the additional fuzzy memo matching code path and if that run fails to give to exact same result we expect from the "simple" fuzzy date match it would mean that either fuzzy memo matching has unwanted "side effects" or we forgot to correctly adjust this test for foreseen and wanted new "side effects".

That said, as below, you convinced me that its better for now to remove those parts that relate to fuzzy memo and to add them again later when it's actually implemented.
KISS :)
I'll prepare the updating commits during the next days.

This comment has been minimized.

@hsoft

hsoft Nov 24, 2017

Owner

I agree with your logic with regards to parametrization. What confused me was that the flag had no effect yet and I wasn't sure whether the results were supposed to be the same regardless of the flag value. If it is, then this makes sense.

def test_match_for_each_existing_in_range(do_refine_matches):
# Verify a match is created for each existing in the date range
plugin = FuzzyDateBind()
num = randint(1, 50) # to limit the runtime of the test

This comment has been minimized.

@hsoft

hsoft Oct 28, 2017

Owner

I'm open to discussion on this point, but I tend to view random elements in tests negatively. The role of tests is to ensure the absence of regressions, not to discover new bugs. Fuzzers to a better job at the latter (I've never run moneyguru through a fuzzer though...). I tend to think that for tests, repeatability is more important. I'd be inclined to suggest that we simply pick a value and hardcode it in the test.

This comment has been minimized.

@hsoft

hsoft Oct 30, 2017

Owner

It's very funny because I've just stumbled on tests code I've recently written myself on another project that uses random test values: https://github.com/hsoft/icemu/blob/master/tests/test_counters.py#L16 . I'm a bit ashamed...

You know what? Forget about my previous comment. Random elements in tests are fine.

This comment has been minimized.

@tuxlifan

tuxlifan Nov 23, 2017

Contributor

Wow, that looks like a very cool project! Can one also emulate relays with it?

On the random issue: I think my original intention was to do some "micro-fuzzing" since I could not quickly come up with a profound static number.
Thanks to this discussion I realized that I actually had missed the num=0 edge case. Now testing with a loop and num in [0, 1, 2, randint(3, 50)] takes on my machine the same ~0.03 seconds for this file as before and it feels much better.
With range(0, 50) that would increase to ~0.21s and with range(0, 150) to even ~1.7s on this machine which seems a bit high considering the amount of time a full test run uses up already and the marginal benefit (if any).

This comment has been minimized.

@hsoft

hsoft Nov 24, 2017

Owner

edge cases + random sound good to me.

As to relays on ICemu, they aren't implemented yet but it should be relatively easy to add. I haven't used any in any of my hobby project so I haven't added them yet. I'm currently rewriting the whole thing in C though, speed is more of an issue than I first though: https://github.com/hsoft/icemuc

eq_(r[1], 'i1')
# ## Disabled until FuzzyMemoBind re-added

This comment has been minimized.

@hsoft

hsoft Oct 28, 2017

Owner

Since we already have this code somewhere else, let's not burden ourselves with comment noise and re-add it when we actually need it.

This comment has been minimized.

@tuxlifan

tuxlifan Nov 22, 2017

Contributor

agreed.

@mark.skipif(FuzzyDateBind.DELTA_T.days < 2,
reason="Need FuzzyDateBind.DELTA_T >= 2 to test due to static special dates.")
@mark.parametrize("do_refine_matches", [(False), (True)])
def test_unix_epoch(do_refine_matches):

This comment has been minimized.

@hsoft

hsoft Oct 28, 2017

Owner

I don't understand the purpose of this test. The code that you test doesn't have any specific handling for this special case, so you're effectively testing python's datetime module. Assuming we want to keep this test around, couldn't we test the occurrence of the overflow bug on a system with datetime directly, without going through FuzzyDateBind? The test would be simpler.

This comment has been minimized.

@tuxlifan

tuxlifan Nov 23, 2017

Contributor

I believe it is important to have this test so that in the event that someone would have issues with this plugin not working correctly because of a broken C library/python installation we could find out (eventually, when/if they run the test...).

We can indeed make a direct test of datetime functionality used and drastically simplify it (and get rid of the @mark.skipif in the process :-)).

@hsoft

Code-wise, this looks good to me. Import mechanics in moneyguru is a bit complicated and you went through it well.

I haven't tested it functionally yet, will do later.

@tuxlifan

This comment has been minimized.

Contributor

tuxlifan commented Oct 28, 2017

Thank you @hsoft; I'm planning to come back to this in detail around 17 November-ish, sorry for the delay.

@tuxlifan tuxlifan referenced this pull request Oct 28, 2017

Closed

continue PR #496 #5

@hsoft hsoft self-assigned this Oct 30, 2017

@hsoft

This comment has been minimized.

Owner

hsoft commented Nov 4, 2017

Note: the PR is missing the inclusion of the plugin reference in plugin/__init__.py.

@hsoft

This comment has been minimized.

Owner

hsoft commented Nov 4, 2017

I've tested it functionally and it works well, good job!

It's only a matter of addressing my previous comments and it's ready to merge.

Would you like to write documentation for this new functionality? It the plugin is undocumented, it risks not being used.

tuxlifan added some commits Nov 22, 2017

Set logging.DEBUG level for all tests.
When a test fails any debug() or higher urgency logging that was captured
during its pytest run will be output for easier diagnostics.
Add comment to also extend PENALTIES dict if want a DELTA_T > 7 days
Safeguarding the key lookup with try/except would be complex logically and
DELTA_T and PENALTIES should be set up by considering them together anyway.
Make test_match_for_each_existing_in_range more deterministic, test e…
…dge case

Added testing of num=0 edge case. Always test with num = 0, 1, 2 plus a randint().
@tuxlifan

This comment has been minimized.

Contributor

tuxlifan commented Nov 23, 2017

The documentation would be with a new section in help/*/plugins.rst, right? I'll give it a try.

@hsoft

This comment has been minimized.

Owner

hsoft commented Nov 24, 2017

@tuxlifan en will suffice.

BASE_CONFIDENCE = 0.75 # could be increased when implement memo matching / associations...
DELTA_T = datetime.timedelta(4) # 4 days; you must extend PENALTIES if DELTA_T > 7 days or a KeyError could occur!
# PENALTIES must have keys for all integers in the inclusive range [ -DELTA_T.days; +DELTA_T.days ]

This comment has been minimized.

@hsoft

hsoft Nov 24, 2017

Owner

or you could do something like PENALTIES.get(delta, max(PENALTIES.values()) to return the maximum possible penalty value when out of range.

for r in result:
eq_(r[0], 'random entry')
eq_(r[1], 'i1')
for num in [0, 1, 2, randint(3, 50)]: # TODO: Use fuzzer. Arbitrarily limited to limit the test runtime

This comment has been minimized.

@hsoft

hsoft Nov 24, 2017

Owner

could also be parametrized

@hsoft

This comment has been minimized.

Owner

hsoft commented Nov 25, 2017

Ready to merge. My last two comments are just tips for future code. I don't actually think you need to change anything. I let you come back with the docs and we're merging. Good job, thanks!

@tuxlifan

This comment has been minimized.

Contributor

tuxlifan commented Dec 1, 2017

@hsoft: ready :)

Test failure is unrelated, in a test for BOCProviderPlugin.

@hsoft hsoft merged commit 2eaa9f3 into hsoft:master Dec 1, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment