-
Notifications
You must be signed in to change notification settings - Fork 63
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fix various code issues to run the example files.
remove dependencies on stocks.df fix fitTfsm on missing beta rownames
- Loading branch information
Showing
11 changed files
with
40 additions
and
27 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
8ef47f0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to see.
8ef47f0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @kecoli!
Thanks for your commit. Since the commit message is very generic, I'm trying to understand why some of these updates were made.I'm fairly certain they mostly fall under #43 and should be tagged as such for others to see under that issue.
Some of this is making sense, like swapping out data for others (although I believe we are eliminating the
factorDataSetDjia5Yrs
?), but others are less clear. Perhaps you can help.For example, why did you add the extra code to
fitTsfm.R
? I am aware of the issues with merge renaming columns that containing spaces, so that makes sense, but those examples were working for me locally. I'm curious, what error message were you getting, that prompted this, if any.Also, why did you wrap the example for
fmmcSemiParam.R
in `dontrun{ } ? If the example doesn't work, it should be fixed and/or removed. Otherwise, people will try to use the documented example anyway, and if we know its going to break, why would we leave it there to frustrate users? If we want to save the code to work on later, it should tagged as an issue and saved for later, as @martinrd3D did in #58. If we leave it wrapped in dontrun, it won't show up on checks and we will forget its a problem...until some frustrated user creates a new issue down the road, which I think you can agree, is not ideal.Thanks,
Justin
8ef47f0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
8ef47f0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I figured out most of these. The
fitTsfm.R
line was helpful, and looks like another example fails without it. It appears I removed it in a previous commit working too late! My apologies, and thanks for catching this @kecoli !Other than that, the fmmcSemiParam.R example remains. I'll see what's going on with that and create an issue if needed
8ef47f0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.