Ready for documentation work #16
Merged
Conversation
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
Literally commited the output of `styler::style_pkg()`.
This should fix check issues compiling on Windows. Apparently, the former is not a standard type, but the latter is (https://stackoverflow.com/a/5678057/1169233).
Some functions had names containing periods, which confuses with S3 dot notation. Others had different cases (snake case, camel case), which may confuse users.
Facilitates development and maintenance while preventing merge conflicts
This avoids confusion with dot-notation.
8 tasks
|
Hello Waldir, Error: Not compatible with requested type: [type=S4; target=double]. Theo |
|
Hello Waldir,I have merged the new PR and has corrected the error I found with the CV_MADMMplasso. It was actually the predict.MADMMplasso that had a little issue because of the output from model_p function, which is now a cpp function.
There is one observation so far with the recent version. It seems the C++ is now very slower than the R when I run with the example on github (setting p=500 and nlambda=50). I don't know wether it is due to the back and forth between C++ and R for each lambda.
You can check this by calling the MADMMplasso function and set legacy=T or F (I have included this in the call).
I hope to complete the documentation soon.
Best regards,Theo
Theophilus Quachie Asenso, PhDPost-doctoral research fellowUniversity of Oslo, Norway+4797358955
On Thursday, November 30, 2023 at 07:21:37 AM GMT+1, Waldir Leoncio ***@***.***> wrote:
@wleoncio requested your review on: #16 Ready for documentation work.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because your review was requested.Message ID: ***@***.***>
|
|
Thanks, I'll open an issue from your comment above regarding the performance. We can work on it at the same time as the documentation. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi Theo,
This PR contains all the changes we discussed yesterday. Please review and test the code carefully, noticing that some of your external scripts may not work since some function changed names (capitalization was standardized).
Once this is merged, all we need to submit to cran is to work on issues #5 and #12, and possibly #15. Decided to split this into fewer PRs than what I said yesterday because it felt less confusing to me to do it like this, hope that's ok. :)
Summary of commits
uinttounsigned int(Minor documentation adjustments for CRAN #13)cv.MADMMplassoclass ascv_MADMMplasso(Minor documentation adjustments for CRAN #13)/inst/examples(Minor documentation adjustments for CRAN #13)To test this PR
Please run the code below to install and review this version of MADMMplasso: