Skip to content
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

Update params in user facing fxns #134

Merged
merged 27 commits into from
Jul 12, 2024
Merged

Update params in user facing fxns #134

merged 27 commits into from
Jul 12, 2024

Conversation

Daenarys8
Copy link
Contributor

This commit updates parameters of user functions.

@Daenarys8 Daenarys8 requested a review from TuomasBorman July 8, 2024 04:19
Copy link
Contributor

@TuomasBorman TuomasBorman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial thoughts

R/plotAbundance.R Outdated Show resolved Hide resolved
R/plotAbundanceDensity.R Outdated Show resolved Hide resolved
R/plotPrevalence.R Outdated Show resolved Hide resolved
R/plotAbundance.R Outdated Show resolved Hide resolved
@TuomasBorman
Copy link
Contributor

plotPrevalence has parameter called label. plotGraph has the same parameter called show.label. I think they are doing the same thing, but "label" is little bit more unclear. -> can you check that they do the same thing and rename them to show.label?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that you have updated only these tests. I guess the other functions should be also updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok noted.

R/plotPrevalence.R Outdated Show resolved Hide resolved
Signed-off-by: Daena Rys <[email protected]>
R/plotTree.R Outdated Show resolved Hide resolved
@Daenarys8
Copy link
Contributor Author

plotPrevalence has parameter called label. plotGraph has the same parameter called show.label. I think they are doing the same thing, but "label" is little bit more unclear. -> can you check that they do the same thing and rename them to show.label?

Hmm, label focuses on rownames while show.label handles logical vectors for selective labeling and matches character vectors to node data columns i.e label is used to specify which labels from the rownames of the data should be selected and show.label controls whether and which tip labels are plotted.

Should I go ahead and rename to show.lable?

@TuomasBorman
Copy link
Contributor

plotPrevalence has parameter called label. plotGraph has the same parameter called show.label. I think they are doing the same thing, but "label" is little bit more unclear. -> can you check that they do the same thing and rename them to show.label?

Hmm, label focuses on rownames while show.label handles logical vectors for selective labeling and matches character vectors to node data columns i.e label is used to specify which labels from the rownames of the data should be selected and show.label controls whether and which tip labels are plotted.

Should I go ahead and rename to show.lable?

I think the idea is still the same; select labels that are going to be shown in the plot? Right? I think then the names should be equal (at least for me it was not intuitive what label do. For show.label it is much more clear what it does; controls labels)

@Daenarys8
Copy link
Contributor Author

plotPrevalence has parameter called label. plotGraph has the same parameter called show.label. I think they are doing the same thing, but "label" is little bit more unclear. -> can you check that they do the same thing and rename them to show.label?

Hmm, label focuses on rownames while show.label handles logical vectors for selective labeling and matches character vectors to node data columns i.e label is used to specify which labels from the rownames of the data should be selected and show.label controls whether and which tip labels are plotted.
Should I go ahead and rename to show.lable?

I think the idea is still the same; select labels that are going to be shown in the plot? Right? I think then the names should be equal (at least for me it was not intuitive what label do. For show.label it is much more clear what it does; controls labels)

I agree. show.label is more intuitive. Will add the change.

@TuomasBorman
Copy link
Contributor

I checked the unit tests. There are many tests that test internal functions, and parameter names of internal functions are not changed if they cannot be used by user. Unit tests are updated and should work after deprecated parameter names are removed in the future.

@TuomasBorman TuomasBorman merged commit c1c277a into devel Jul 12, 2024
1 of 3 checks passed
@TuomasBorman TuomasBorman deleted the pv2 branch July 12, 2024 21:04
@Daenarys8 Daenarys8 mentioned this pull request Jul 13, 2024
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