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

Minimum marktkapitalisatie "off by one" error #54

Open
roytomeij opened this issue May 24, 2024 · 4 comments
Open

Minimum marktkapitalisatie "off by one" error #54

roytomeij opened this issue May 24, 2024 · 4 comments
Labels
improvement An improvement on an existing feature

Comments

@roytomeij
Copy link

Voor de minimum marktkapitalisatie wordt gebruikt gemaakt van marktkapitalisatie > waarde terwijl dit marktkapitalisatie >= waarde zou moeten zijn. Nu worden fondsen met een marktkapitalisatie van 93% alleen getoond wanneer de ingevoerde waarde lager is dan 93.

@jakedane
Copy link
Contributor

It does use >=. See at https://github.com/nicwortel/indexfondsenvergelijken.nl/blob/master/src/index.ts#L57 (not the =>, the >= after):

    portfolios = portfolios.filter((portfolio: Portfolio) => portfolio.getMarketCapPercentage().getPercentage() >= minimumMarketCap);

I think what you describe is caused by rounding. For example the 3 NT funds have a market cap of 92.8681203778817%. See at https://github.com/nicwortel/indexfondsenvergelijken.nl/blob/master/data/indices.json#L121. That is rounded up to 93% for display but it compares that 92.8681203778817% with the market cap filter, so you have to set it to 92% to match.

@nicwortel
Copy link
Owner

Good catch @jakedane, that seems to be the case indeed.

However I can understand the confusion as most people will not look into the data files, so I'm open for suggestions on how to make this more intuitive. For that reason I will leave this issue open.

@jakedane
Copy link
Contributor

jakedane commented May 28, 2024

Given that the "Minimum marktkapitalisatie" field on the site allows only integer numbers, maybe change the filter code from:

portfolio.getMarketCapPercentage().getPercentage() >= minimumMarketCap

to:

portfolio.getMarketCapPercentage().getPercentage() >= minimumMarketCap - 0.5

Basically a "Minimum marktkapitalisatie" of 93 would match portfolios with a market cap that rounds to at least 93. For example the 3 NT funds at market cap 92.8681203778817, which the site shows rounded as 93, would now match if the filter is set to 93.

@nicwortel nicwortel added the improvement An improvement on an existing feature label Jul 7, 2024
@roytomeij
Copy link
Author

Sorry, only seeing this one now. Good catch @jakedane! Lazy me hadn't looked at the source code, so I'm glad others are better open-source citizens than me :)

I'd round up the funds' market cap to the nearest integer instead of subtracting 0.5 from the input integer. That seems like a more failsafe way to handle these edge cases.

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

No branches or pull requests

3 participants