279_getquote resillient to invalid symbols #282

ethanbsmith · 2019-11-03T17:12:09Z

getQuote would fail if the source did not recognize any of the supplied symbols. this was particularly problematic on large batches where a single failure would cause a stop and there was no way to diagnose which symbol had caused the problem

all getQuote paths now support ";" delimited inputs
All getQuote paths now return NA rows for missing quotes

all getquote paths now support ";" delimited inputs and return NA for missing quotes

ethanbsmith · 2019-11-03T17:23:09Z

ready for review. could not add tests for tiingo and av as these require a key

joshuaulrich

I reviewed and left some comments. Let me know if you would like me to make the changes I suggested.

joshuaulrich · 2019-11-04T11:56:46Z

R/getQuote.R

@@ -369,11 +377,14 @@ getQuote.av <- function(Symbols, api.key, ...) {
  }

  colnames(r) <- gsub("(^[[:alpha:]])", "\\U\\1", colnames(r), perl = TRUE)
-  r[, "Trade Time"] <- r[, "LastSaleTimestamp"]
+  colnames(r)[which(colnames(r) == "LastsaleTimeStamp")] <- "Trade Time"


I assume you're doing this so the "Trade Time" column appears first, instead of last.

This introduces a bug. The column returned by Tiingo is "LastSaleTimestamp" (note "Sale" is initial capitalized).

Also, the call to which() is unnecessary. It's okay to use the logical vector returned by ==.

A more robust solution would be to leave L372 as-is, and follow it with r[, "LastSaleTimestamp"] <- NULL to remove the "LastSaleTimestamp" column.

joshuaulrich · 2019-11-04T11:57:36Z

R/getQuote.R

+    sq <- merge(list(symbol = Symbols), sq, by = "symbol", all = TRUE)
+  }
+
+    # Extract user-requested columns. Convert to list to avoid


Please be careful of accidentally introducing white space changes like this.

joshuaulrich · 2019-11-04T11:58:08Z

R/getQuote.R

@@ -79,6 +83,7 @@ function(Symbols,what=standardQuote(),...) {

  # Add the trade time and setNames() on other elements
  qflist <- c(list(regularMarketTime = Qposix), setNames(qflist, QF))
+


Another white space change. This one clutters the diff a little bit.

joshuaulrich · 2019-11-04T11:59:55Z

R/getQuote.R

@@ -280,6 +285,7 @@ getQuote.av <- function(Symbols, api.key, ...) {
                "?function=BATCH_STOCK_QUOTES",
                "&apikey=", api.key,
                "&symbols=")
+  Symbols <- unlist(strsplit(Symbols,';'))


It's good practice to only have one functional change per commit, so this should be in another commit. I like that you made this change though! Can you think of a way to add a unit test?

joshuaulrich · 2019-11-04T12:00:05Z

R/getQuote.R

@@ -326,6 +332,8 @@ getQuote.av <- function(Symbols, api.key, ...) {
         "Registration at https://api.tiingo.com/.", call. = FALSE)
  }

+  Symbols <- unlist(strsplit(Symbols,';'))


It's good practice to only have one functional change per commit, so this should be in another commit.

ethanbsmith · 2019-11-04T12:39:04Z

Good points. I won’t be able to get to this till after market closes today. If you’re on a roll, I wont be offended if you make the changes Also, on the Symbols <- unlist(strsplit(Symbols,',')) changes, I think that processing can be moved up to the master getQuote function. I can do a separate ticket/pr to cover that if you agree on the approach Not sure how to unit test the av and tiingo functions without a key though

ethanbsmith · 2019-11-04T23:12:06Z

ok, i made some changes. i think the diffs should be much cleaner now. Also, i think it's set up to consolidate the strsplit and merge code into the master function

joshuaulrich · 2019-11-04T23:44:20Z

I think you removed your functional changes, and some code that was in master (before your PR). I'll fix and merge tomorrow morning, at the latest. Put your symbol splitting code in another branch, and I'll make sure it gets merged too.

…

On Mon, Nov 4, 2019, 5:12 PM Ethan Smith ***@***.***> wrote: ok, i made some changes. i think the diffs should be much cleaner now. Also, i think it's set up to consolidate the strsplit and merge code into the master function — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#282?email_source=notifications&email_token=AAHZZWP3Y465O5VTJHFUDMDQSCT4PA5CNFSM4JIMGC3KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDBBJDY#issuecomment-549590159>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHZZWLSF2J2N44KNF7XV53QSCT4PANCNFSM4JIMGC3A> .

ethanbsmith · 2019-11-04T23:51:35Z

i realized the merge call wasn't needed. you can just use the symbol names returned from the server. If we want to ensure that the output includes NA rows for input symbols, which isn't a bad idea, that can be centralized into the main function, after the getting the dispatched results. If you want that, I can do at the same time as the symbol splitting. I'll do that after this PR gets merged so the conflicts don't get complicated. I already opened an issue to track that

joshuaulrich · 2019-11-06T12:59:33Z

If we want to ensure that the output includes NA rows for input symbols, which isn't a bad idea, that can be centralized into the main function, after the getting the dispatched results.

We do want that, because that's the existing behavior. Removing it would break backward compatibility. Moving that to the main function should be in a separate PR, so I'm going to revert a71e1e5 in this branch.

I'm going to leave the Symbols <- unlist(strsplit(Symbols,';')) because I'm not sure your updates will work without them.

And I'm not going to make any changes until I understand my confusion, described below.

I just noticed that the getQuote functions still error if all the symbols are not valid. Your initial description said:

getQuote would fail if the source did not recognize any of the supplied symbols.

(Emphasis mine). I expected these calls to return objects that only contained missing values, but they still error.

quantmod::getQuote(c("FOO", "WYSIWYG"), src = "av", api.key = keyAV)
quantmod::getQuote(c("FOO", "WYSIWYG"), src = "tiingo", api.key = keyTiingo)
quantmod::getQuote(c("FOO", "WYSIWYG"), src = "yahoo")

Am I missing something? Maybe I don't understand the initial problem as well as I thought...

And another thing...

ready for review. could not add tests for tiingo and av as these require a key

My keys for Alpha Vantage and Tiingo are on TravisCI. They are stored in environment variables QUANTMOD_AV_API_KEY and QUANTMOD_TIINGO_API_KEY, respectively. Here's an example of how to use them. Notice that the environment variables are spelled wrong though...

quantmod/tests/test_getSymbols.R

Lines 4 to 5 in 1739987

    
           apikey <- Sys.getenv("AV_API_KEY") 
        
           if (apikey != "") {

So you can write tests and they will run on TravisCI when you push. But please don't use them just yet... as I was testing this morning, I noticed that error messages contain the key in plain text. The TravisCI logs are public, and I don't want my keys displayed.

ethanbsmith · 2019-11-06T13:24:57Z

the centralized code is already in PR #285. it might simplify this PR if that one gets accepted first. not sure.

I just noticed that the getQuote functions still error if all the symbols are not valid.

(Emphasis mine). Interesting. I've never encountered that myself and didn't test for it. It's an odd scenario, but probably one we should account for as well. Are you ok if we handle that in this PR or do you want it broken out?

Thx for the info on the keys. i will add test for av and tiingo

joshuaulrich · 2019-11-06T13:52:52Z

Thx for the info on the keys. i will add test for av and tiingo

To reiterate, please don't push those tests until I'm convinced my keys won't be publicly visible in an error message.

I looked at the issue again, and I see that your example was for getQuote.yahoo(). I confirm that function throws an error if any symbol is not valid. getQuote.av() and getQuote.tiingo() add the rows for any missing symbols. That's why I was confused. It makes sense now.

I'll take a look at this again after market hours today.

ethanbsmith · 2019-11-06T20:46:13Z

I just noticed that the getQuote functions still error if all the symbols are not valid.

been stewing on this in the background all day. not as straight forward as I originally thought. it's fairly innocuous in the simple case of 1 or 2 tickers, but what about 2000 or 10000? if every single ticker you passed in is rejected, something is probably wrong. Sort of seems like a different use case from a disagreement on a subset of tickers. Not sure I have a strong view on "correct" behavior either way on this aspect and would support whatever you decide on

This reverts commit a71e1e5.

Also standardize symbols data.frame that is merged with data returned by the data source.

This reverts commit c5f0aec.

getquote resillient to invalid symbols

7ce1049

all getquote paths now support ";" delimited inputs and return NA for missing quotes

ethanbsmith changed the title ~~getquote resillient to invalid symbols~~ 279_getquote resillient to invalid symbols Nov 3, 2019

ethanbsmith marked this pull request as ready for review November 3, 2019 17:15

joshuaulrich reviewed Nov 4, 2019

View reviewed changes

ethanbsmith added 6 commits November 4, 2019 16:00

Update getQuote.R

a71e1e5

Update getQuote.R

259247b

Update getQuote.R

586e665

Update getQuote.R

dc073e3

Update getQuote.R

12e7a46

Update getQuote.R

1a9f748

ethanbsmith and others added 7 commits November 7, 2019 05:34

Update getQuote.R

c5f0aec

Revert "Update getQuote.R"

338ea77

This reverts commit a71e1e5.

fixup! getquote resillient to invalid symbols

c730936

fixup! getquote resillient to invalid symbols

454c9fc

Add Alpha Vantage and Tiingo getQuote tests

c6f1e77

Also standardize symbols data.frame that is merged with data returned by the data source.

Revert "Update getQuote.R"

b9196ed

This reverts commit c5f0aec.

Add comments

5e9ce4b

joshuaulrich merged commit 4b93bb2 into joshuaulrich:master Nov 18, 2019

joshuaulrich mentioned this pull request Nov 18, 2019

getQuote fails if any symbols are not found #279

Closed

ethanbsmith mentioned this pull request Nov 19, 2019

getQuote and obsolete Warrant #241

Closed

joshuaulrich added this to the Release 0.4-16 milestone Feb 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

279_getquote resillient to invalid symbols #282

279_getquote resillient to invalid symbols #282

ethanbsmith commented Nov 3, 2019

ethanbsmith commented Nov 3, 2019

joshuaulrich left a comment

joshuaulrich Nov 4, 2019

joshuaulrich Nov 4, 2019

joshuaulrich Nov 4, 2019

joshuaulrich Nov 4, 2019

joshuaulrich Nov 4, 2019

joshuaulrich Nov 4, 2019

ethanbsmith commented Nov 4, 2019 via email

ethanbsmith commented Nov 4, 2019

joshuaulrich commented Nov 4, 2019 via email

ethanbsmith commented Nov 4, 2019

joshuaulrich commented Nov 6, 2019 •

edited

Loading

ethanbsmith commented Nov 6, 2019

joshuaulrich commented Nov 6, 2019

ethanbsmith commented Nov 6, 2019

		@@ -79,6 +83,7 @@ function(Symbols,what=standardQuote(),...) {

		# Add the trade time and setNames() on other elements
		qflist <- c(list(regularMarketTime = Qposix), setNames(qflist, QF))

279_getquote resillient to invalid symbols #282

279_getquote resillient to invalid symbols #282

Conversation

ethanbsmith commented Nov 3, 2019

ethanbsmith commented Nov 3, 2019

joshuaulrich left a comment

Choose a reason for hiding this comment

joshuaulrich Nov 4, 2019

Choose a reason for hiding this comment

joshuaulrich Nov 4, 2019

Choose a reason for hiding this comment

joshuaulrich Nov 4, 2019

Choose a reason for hiding this comment

joshuaulrich Nov 4, 2019

Choose a reason for hiding this comment

joshuaulrich Nov 4, 2019

Choose a reason for hiding this comment

joshuaulrich Nov 4, 2019

Choose a reason for hiding this comment

ethanbsmith commented Nov 4, 2019 via email

ethanbsmith commented Nov 4, 2019

joshuaulrich commented Nov 4, 2019 via email

ethanbsmith commented Nov 4, 2019

joshuaulrich commented Nov 6, 2019 • edited Loading

ethanbsmith commented Nov 6, 2019

joshuaulrich commented Nov 6, 2019

ethanbsmith commented Nov 6, 2019

joshuaulrich commented Nov 6, 2019 •

edited

Loading