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

Improved column matching for OHLCVA #306

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nocodehummel
Copy link

The functions to match and extract OHLCVA columns return information about OHLCVA data in the input. The implemented logic aims to find the right column also when columns with similar names exist.

This change resolves issues in matching the correct column(s). Known bugs are when the column name has multiple similar matches. E.g. the name is similar to the symbol.

The solution is first attempting an exact column name match using which (this was done with attr). When this fails the fallback is to search for column name(s) ending with the matching term using grep.

This change also removes code duplication that existed between related functions (e.g. has.Op and Op), since each pair related functions attempted to match column names with grep.

As requested test coverage is added with tinytest.

The functions to match and extract OHLCVA columns return information about OHLCVA data in the input.
The implemented logic aims to find the right column also when columns with similar names exist.

This change resolves issues in matching the correct column(s).
Known bugs are when the column name has multiple similar matches.
E.g. the name is similar to the `symbol`.

The solution is first attempting an exact column name match.
When this fails the fallback is to search for column name(s) ending with the matching term.

This change also removes code duplication that existed between related functions (e.g. `has.Op` and `Op`),
since both functions attempted to match column names with `grep`.

As requested test coverage is added with tinytest.
@nocodehummel
Copy link
Author

In response to @joshuaulrich comments in #305 .

  1. I started by trying to find test case(s) to return a result from the column attribute handling (attr(x, "Op")), but could not verify the functionality with a test case. Do you know a data structure for which this should return a result? What is the expected behaviour?

  2. n <- which(colnames(x) %in% c("Op", "Open")) is introduced to replace attr(x, "Op"), plus covering the sample data where column name is "Open". This passed my test cases, where the name matches exactly. Since I could not verify 1) with a test case, I may have jumped to conclusion about what the expected behaviour is.

  3. yes, agreed - I started by adding the tests! Therefor 1) and 2). Btw; thanks for pointing out tinytest, started to use it in my own work now.

Looking forward to your feedback on the PR.

@ethanbsmith ethanbsmith mentioned this pull request Oct 22, 2024
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.

1 participant