M farouk b patch 3 #90
Open
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
michaelweylandt
requested changes
Apr 9, 2019
R/save.R
Outdated
| @@ -1,6 +1,6 @@ | |||
| #' @rdname plot_carp | |||
| #' @export | |||
| saveviz <- function(x, ...) { | |||
| saveviz <- function(x,frames_number,frame_rate, ...) { | |||
There was a problem hiding this comment.
I think these should be arguments of the saveviz.CARP and saveviz.CBASS methods only - not the generic.
R/save.R
Outdated
| @@ -1,6 +1,6 @@ | |||
| #' @rdname plot_carp | |||
| #' @export | |||
| saveviz <- function(x, ...) { | |||
| saveviz <- function(x,frames_number,frame_rate, ...) { | |||
There was a problem hiding this comment.
These need to be documented (for the methods) -- see how the other arguments are documented below.
R/save.R
Outdated
| @@ -1,6 +1,6 @@ | |||
| #' @rdname plot_carp | |||
| #' @export | |||
| saveviz <- function(x, ...) { | |||
| saveviz <- function(x,frames_number,frame_rate, ...) { | |||
There was a problem hiding this comment.
These also need default values.
|
Hi @MFaroukB, as I commented inline, there are some changes re: default values and documentations that should be fixed before we can merge this. They should be minor (I hope!) but feel free to ping me with questions.
|
|
Yes, I will get them done in a second.
I have just been working on the application and didn't see the reply the
moment it was sent.
…On Mon, Apr 8, 2019, 21:05 Michael Weylandt ***@***.***> wrote:
Hi @MFaroukB <https://github.com/MFaroukB>, as I commented inline, there
are some changes re: default values and documentations that should be fixed
before we can merge this.
They should be minor (I hope!) but feel free to ping me with questions.
- M
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#90 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AvEP3T6ioFtl87fpY_g0h8ACSaEAsxfTks5ve_V8gaJpZM4cjS5R>
.
|
|
The Only problem is the function animate is not even used in saveviz.CBASS
…On Mon, Apr 8, 2019, 21:37 Mahmoud Badawi ***@***.***> wrote:
Yes, I will get them done in a second.
I have just been working on the application and didn't see the reply the
moment it was sent.
On Mon, Apr 8, 2019, 21:05 Michael Weylandt ***@***.***>
wrote:
> Hi @MFaroukB <https://github.com/MFaroukB>, as I commented inline, there
> are some changes re: default values and documentations that should be fixed
> before we can merge this.
>
> They should be minor (I hope!) but feel free to ping me with questions.
>
> - M
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#90 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AvEP3T6ioFtl87fpY_g0h8ACSaEAsxfTks5ve_V8gaJpZM4cjS5R>
> .
>
|
|
I have added 4 commits to my previous patch. I believe it should appear now
for you!
If it didn't, let me know. I will redo it as I did before.
…On Mon, Apr 8, 2019, 21:39 Mahmoud Badawi ***@***.***> wrote:
The Only problem is the function animate is not even used in saveviz.CBASS
On Mon, Apr 8, 2019, 21:37 Mahmoud Badawi ***@***.***> wrote:
> Yes, I will get them done in a second.
>
> I have just been working on the application and didn't see the reply the
> moment it was sent.
>
> On Mon, Apr 8, 2019, 21:05 Michael Weylandt ***@***.***>
> wrote:
>
>> Hi @MFaroukB <https://github.com/MFaroukB>, as I commented inline,
>> there are some changes re: default values and documentations that should be
>> fixed before we can merge this.
>>
>> They should be minor (I hope!) but feel free to ping me with questions.
>>
>> - M
>>
>> —
>> You are receiving this because you were mentioned.
>> Reply to this email directly, view it on GitHub
>> <#90 (comment)>,
>> or mute the thread
>> <https://github.com/notifications/unsubscribe-auth/AvEP3T6ioFtl87fpY_g0h8ACSaEAsxfTks5ve_V8gaJpZM4cjS5R>
>> .
>>
>
|
|
This looks good for the changes for the gganimate based plots ( |
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.
Adding variables to saveviz function to control fps and number of frames.