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

Numerical fixes for UOB importer #70

Closed
kaiwei opened this issue Jul 2, 2023 · 12 comments
Closed

Numerical fixes for UOB importer #70

kaiwei opened this issue Jul 2, 2023 · 12 comments

Comments

@kaiwei
Copy link

kaiwei commented Jul 2, 2023

  1. "Cash Withdrawal" entries should have 2 decimal places even for whole numbers but are currently missing. This affects automatic numerical display inference in Fava
  2. Entries should mirror numerical precision from exported Excel bank source, which are to 2 decimal places. But this seems to be rounded down (now) plus the numerical precision seems to go out to several lengthy number of places (which also somehow crashes fava)
  3. Importer outputs a warning message to terminal line when ran: "WARNING *** file size (92598) not 512 + multiple of sector size (512)"
  4. Last minor nit tweak: Possible to output the larger positive numberical figures, e.g., 1,000, as it is, without commas (i.e, 1000)? Commas rendering should be something adjustable in fava, etc., but not part of the raw beancount output?
@redstreet
Copy link
Owner

redstreet commented Jul 2, 2023

@kaiwei would you mind please attaching these three: anonymized data, expected output, and a minimal config file? This will help debug, for to help turn the debugged cases into unit tests to prevent regressions. Thanks!

@kaiwei
Copy link
Author

kaiwei commented Jul 3, 2023

I had emailed them to you (earlier) the raw actual files (given privacy concerns). Hope that's okay! Minimal config below:

See below

@redstreet
Copy link
Owner

I totally understand, anonymizing data is a pain. But if you could post even minimal anonymized data here that I can commit to the repository as tests (eg: a single transaction for each issue here), that would be of great help in ensuring things don't break in the future.

IMHO, this is a good opportunity to build and commit tests. Continuing to build and make fixes without unit tests rarely has good outcomes, and I'm simply trying to ensure we end up with a solid, stable codebase with these importers.

I hope that makes sense? Happy to discuss more if needed.

@kaiwei
Copy link
Author

kaiwei commented Jul 4, 2023

No problems. I'll get to it for the purpose of the tests, but hopefully the actual files (which were emailed) are useful to you in the meantime.

Appreciate the help as always.

@redstreet
Copy link
Owner

Sounds great, thanks much! If it's okay, I'll wait to receive the data to commit the fix to avoid duplication of effort.

@kaiwei
Copy link
Author

kaiwei commented Jul 5, 2023

Here you go!

uniplus.zip

Simple config
apply_hooks(uobbank.Importer({
'main_account' : 'Assets:Banks:UOB:UNIPLUS',
'account_number' : '1234567890',
'currency' : 'SGD',
'rounding_error' : 'Equity:Rounding-Errors:Imports',
}), [PredictPostings()]),

redstreet added a commit that referenced this issue Jul 5, 2023
- ignore WARNING *** file size (92598) not 512 + multiple of sector size (512)
redstreet added a commit that referenced this issue Jul 5, 2023
- decimal places
  - still needs a full solution. The problem is, petel uses xlwt/xlrd to
    read excel, which use excel datatypes, which we don't want, as we
    want just the string, which we can then pass on to decimal.decimal
  - not sure exactly how decimals are stored in excel

- don't end up with long floats
redstreet added a commit that referenced this issue Jul 5, 2023
- decimal places
  - still needs a full solution. The problem is, petel uses xlwt/xlrd to
    read excel, which use excel datatypes, which we don't want, as we
    want just the string, which we can then pass on to decimal.decimal
  - not sure exactly how decimals are stored in excel

- don't end up with long floats
@redstreet
Copy link
Owner

redstreet commented Jul 5, 2023

  1. "Cash Withdrawal" entries should have 2 decimal places even for whole numbers but are currently missing. This affects automatic numerical display inference in Fava

Fixed. Still needs a full solution. The problem is, petl uses xlwt/xlrd to read excel, which use excel datatypes, which we don't want, as we want just the string, which we can then pass on to decimal.Decimal. Not sure exactly how decimals are stored in excel

It should have a single digit after the decimal. Please let me know if this solves your issue?

  1. Entries should mirror numerical precision from exported Excel bank source, which are to 2 decimal places. But this seems to be rounded down (now) plus the numerical precision seems to go out to several lengthy number of places (which also somehow crashes fava)

Fixed.

  1. Importer outputs a warning message to terminal line when ran: "WARNING *** file size (92598) not 512 + multiple of sector size (512)"

Fixed.

  1. Last minor nit tweak: Possible to output the larger positive numberical figures, e.g., 1,000, as it is, without commas (i.e, 1000)? Commas rendering should be something adjustable in fava, etc., but not part of the raw beancount output?

Thousands separator-in-source seem to be an issue of personal preference. It improves readability of the source, and some like it for that, particularly if they tend to look at their source a lot. Others don't or don't care.

I'll leave it the way it is to have it remain consistent with beancount_reds_importers. But changing this at your end is trivial: simply run use autobean-format on your source, or pipe the import through it:

autobean-format foo.bean --thousands-separator=add --recursive --output-mode inplace

redstreet added a commit that referenced this issue Jul 5, 2023
- decimal places
  - still needs a full solution. The problem is, petel uses xlwt/xlrd to
    read excel, which use excel datatypes, which we don't want, as we
    want just the string, which we can then pass on to decimal.decimal
  - not sure exactly how decimals are stored in excel

- don't end up with long floats
@kaiwei
Copy link
Author

kaiwei commented Jul 6, 2023

Thanks for the autobean recommendation! It's a great formatter!

All issues seems resolved for now but instead of a single digit after the decimal, can we pad it to two instead, so that it ends as 1.00 SGD or 1.30 SGD, etc. instead of 1.0 SGD and 1.3 SGD, given dollars and cents are up to two decimals and this also mirrors the root excel file.

Thanks again for the help!

@redstreet
Copy link
Owner

That's the part above I referred to as arising from petl using xlwt/xlrd to read excel, and not yet fully resolved. It might require a bit of digging and time to figure out. I'll do it when I get some time next, but it might be a while.

@kaiwei
Copy link
Author

kaiwei commented Jul 7, 2023

Aha got it. Thank you for the help!

@redstreet
Copy link
Owner

Piping through a simple sed should fix the issue until resolved.

@redstreet
Copy link
Owner

Remaining issue will be covered by #74.

scanta2 pushed a commit to scanta2/beancount_reds_importers that referenced this issue Jan 5, 2024
- ignore WARNING *** file size (92598) not 512 + multiple of sector size (512)
scanta2 pushed a commit to scanta2/beancount_reds_importers that referenced this issue Jan 5, 2024
- decimal places
  - still needs a full solution. The problem is, petel uses xlwt/xlrd to
    read excel, which use excel datatypes, which we don't want, as we
    want just the string, which we can then pass on to decimal.decimal
  - not sure exactly how decimals are stored in excel

- don't end up with long floats
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

No branches or pull requests

2 participants