Skip to content

Remove default n values in epi_slide and epix_slide #167

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

Merged
merged 4 commits into from
Jul 28, 2022

Conversation

kenmawer
Copy link
Contributor

Addresses issue #160.

@brookslogan
Copy link
Contributor

brookslogan commented Jul 27, 2022

Code changes look good to go. Doc changes look good, but see question below. Interestingly, as far as I can tell, the vignettes are all already compatible with this change!

Question: is it worth clarifying in the @param n docs (for epi_slide, epix_slide, $slide) that n must be passed by name?

@brookslogan
Copy link
Contributor

I don't think GitHub recognizes "addresses", but does recognize "closes" and "resolves". Closes #160.

@brookslogan brookslogan changed the title Km slide n fix Remove default n values in epi[x]_slide Jul 27, 2022
@kenmawer kenmawer changed the title Remove default n values in epi[x]_slide Remove default n values in epi_slide Jul 27, 2022
@kenmawer
Copy link
Contributor Author

Note that this is for epi_slide only and not epix_slide.

@brookslogan
Copy link
Contributor

In GitHub's diffs, I see changes in epi_slide, epix_slide, and epi_archive's $slide. Are some of them not supposed to be there?

@kenmawer
Copy link
Contributor Author

These should be here, because we are removing the default n values. Any changes in the documentation, etc. is due to taking out the now outdated info about the default n being 7 where it's specified.

@brookslogan
Copy link
Contributor

Sorry, having trouble loading in context.

Note that this is for epi_slide only and not epix_slide.

Could you please explain this a bit further?

@kenmawer kenmawer changed the title Remove default n values in epi_slide Remove default n values in epi_slide and epix_slide Jul 28, 2022
@brookslogan
Copy link
Contributor

Note from call: removing the n default did apply to both epi slide and epix slide. The change that didn't apply to both was in another PR.

@brookslogan brookslogan merged commit 43a90d4 into cmu-delphi:main Jul 28, 2022
@kenmawer kenmawer deleted the km-slide-n-fix branch August 12, 2022 23:47
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.

2 participants