Plot longitudinal defaults

amanda-hi commented 11 months ago

Overview of Pull Request

Other plotting functions in SomaPlotr take quoted variable names (typically column name strings) as function arguments. However, plotLongitudinal() takes only unquoted variable names. To be consistent, I modified plotLongitudinal() to take both quoted and unquoted arguments, and updated the documentation accordingly.

I realize that this will be a breaking change - I can incorporate additional logic to allow both quoted/unquoted arguments for a period of time, so we can deprecate the unquote arg functionality.

I also added unit tests for plotLongitudinal().

Possible ideas to consider: do the same thing to plotVolcano() and plotVolcanoHTML(). Is there a reason why these 3 functions accept unquoted var names as arguments, while the rest of the functions require the arguments to be quoted?

Change type

Please check the relevant box(es):

[ ] New feature (no API change; @amanda-hi, @stufield)
[ ] Bug fix (no API change -> fixes an issue; @amanda-hi, @stufield)
[x] Breaking change (fix or feature with API change; @amanda-hi, @stufield)
[ ] Documentation update (@SLbmangum, @kmurugesan14)
[ ] Package maintenance (structural, non-code, non-breaking changes; @stufield)
[ ] README change (@SLbmangum, @kmurugesan14, @nmcnabbSL)
[ ] LICENSE changes (@SLbmangum)

Choose reviewer(s)

[ ] @amanda-hi
[x] @stufield
[ ] @SLbmangum
[ ] @kmurugesan14
[ ] @nmcnabbSL

Reviewer by Department

Department	Reviewer	Change Type
Bioinformatics	@amanda-hi	Code, bugs, features
	@stufield	bugs, features
Legal	@SLbmangum	LICENSE
Product	@kmurugesan14	Documentation
Regulatory	@nmcnabbSL	Documentation

codecov-commenter commented 11 months ago

Welcome to Codecov :tada:

Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment.

Thanks for integrating Codecov - We've got you covered :open_umbrella:

stufield commented 10 months ago

Not sure this is premature or not, but I think I saw a few test warnings for the typical dplyr -> any_of() or all_of() warning in the new tests for plotLongitudinal(). Delete this comment if already on your radar, just didn't want it to slip through unnoticed.

amanda-hi commented 9 months ago

Not sure this is premature or not, but I think I saw a few test warnings for the typical dplyr -> any_of() or all_of() warning in the new tests for plotLongitudinal(). Delete this comment if already on your radar, just didn't want it to slip through unnoticed.

Those errors are coming from this line and this line. The problem is that the tidyselect functions any_of() and all_of() only work on character vectors. However, because we are passing in symbols to the function, the keys variable used to select columns from the data object is a list (and not a character vector). This is causing the test warnings to be thrown when all_of()/any_of() isn't used where expected. Based on my understanding, we would need to cast the keys variable to a character vector to be able to use all_of()/any_of() and resolve the warnings.

stufield commented 9 months ago

Ah ... ok, so this will be resolved once we figure out the symbols/character/variable piece. I think I can work a bit next week.

SomaLogic / SomaPlotr