Add calculate_cell_cluster_metrics() function

[cansavvy]

Background

This PR is for #10. Tried to follow the context there. But since this is my first PR on this project I might be missing some insights so Im just posting this as a draft first so that someone can check that I'm in the generally right direction.

Summary

This is a function that takes output from sweep_clusters() and runs evals on it. It can run calculate_silhouette() and/or calculate_purity() on all elements of the list of data frames that are outputed from sweep_clusters().

An additional very nit picky thing I have in here but I was very minorly thrown off by the examples in sweep_clusters() being named cluster_df when they are actually lists of data frames and not data frames out right. If you don't like this change no worries. The documentation itself is very clear but I'm just a person who kinda goes straight for the examples first.

Requested feedback

Am I understanding this right? It doesn't seem like this function will be that useful but I trust ya'll have more context and knowledge about the needs of the project than I who just started looking at this stuff last week lol.

Side side question that is also very minor, can we name the data frames in the list according to the combo of their parameters or do we find that would be too clunky? I like names in my lists but this is maybe a cansavvy quirk we don't need to subject everyone else to.

[cansavvy]

Still left to do here:

• Polishing docs (I copied and pasted and edited but IDK) -- did this recently
• Writing tests - This is in Tests for calculate_cell_cluster_metrics() function #29
• Incorporating any feedback - Did this in my most recent commits

[jashapiro]

Thanks for this contribution! My main comment here (aside from my earlier misread) is that I still wonder if we want this function to work on a list, rather than just on a single clustering data frame. The convenience of calculating all the metrics at once makes sense to me, but if we have a function that evaluates a single data frame, then turning that into evaluating the list is a very simple addition of a purrr wrapper, and I think that seems more transparent. But others may disagree!

[sjspielman]

Thanks for starting this!! I left some initial comments here, but before I look more, I have a thought about the use cases for this function - currently it's written to only run on a sweep list, but it makes sense to me to also make this flexible enough to run on single data frame (e.g., not a list of data frames). This means updates related to the input argument sweep_list:

• First, give it a more flexible name... Maybe something like cluster_results? I don't love it, but I'm not sure of how else to communicate that it might be either a list of dfs or df, so really I don't hate it either!
• Second, add a check if it's a data frame and if so, make it a list of length 1 with the given data frame in it. So the code might look like:

if (data frame) { listify it}
else { run the existing stopifnot checks}

• I suppose we'd need another check to determine whether to return the list or the first index from the eval'd df, too, so you might actually structure this code by first defining an is_df variable or so, and using that for both checks (the opening sanity check to transform it into a list to play nicely with purrr, and the final check for what to return.

[cansavvy]

I think all comments have been addressed! Sorry about my very slow speed on this!

[sjspielman]

Welcome back 👋 , never a worry about timing :)

I think this is just about good to go, with the reviews I left (use match.arg and purge the ...).

I'm thinking as well about next steps getting tests in here, since I agree a separate PR is good but stacking doesn't work well in forks as we know! So I think this is the move - what if we remove the @export tag from roxygen in this PR, and in the next PR when tests are added back we add the @export back in? This strikes me as a fair middle ground to merge in an untested function? Open to other ideas if you have them!

[jashapiro]

I'm thinking as well about next steps getting tests in here, since I agree a separate PR is good but stacking doesn't work well in forks as we know! So I think this is the move - what if we remove the @export tag from roxygen in this PR, and in the next PR when tests are added back we add the @export back in? This strikes me as a fair middle ground to merge in an untested function? Open to other ideas if you have them!

I would prefer that the tests be part of the same PR, or at least some basic tests are included. Since @cansavvy already has some tests written in a separate branch, I don't think that is going to slow things down much.

[cansavvy]

I went ahead and just added the tests into here. I didn't want to make the PR too big but I also agree its awkward that there's no tests for these new functions here.

[cansavvy]

Tests in here merged and other suggestions addressed! @sjspielman

[sjspielman]

These tests look good! Can we also add tests to for when only a single metric is specified? For that, you can just check the output data frame column names; don't need to check NAs.

[cansavvy]

Added a test for a single metric!

[cansavvy]

Anything else @sjspielman ?

[sjspielman]

Maybe some miscommunication here - when I said single metric I meant just one of silhouette or purity. But the test you added looks functionally the same as the first test in this regard? The specific clustering parameters don't need to be tested more, but we do want to test the metrics argument.

Actually, along these lines, I'm realizing we don't have an example for this either. Can you add an example the roxygen that specifies metrics = "silhouette"? Thanks!!

[cansavvy]

Maybe some miscommunication here - when I said single metric I meant just one of silhouette or purity. But the test you added looks functionally the same as the first test in this regard? The specific clustering parameters don't need to be tested more, but we do want to test the metrics argument.

Actually, along these lines, I'm realizing we don't have an example for this either. Can you add an example the roxygen that specifies metrics = "silhouette"? Thanks!!

AH okay sorry I misunderstood. I will:

1. Make a test that only looks at silhouette or purity and
2. Add an example to the docs with metrics = "silhouette"

[cansavvy]

These tests are a tad redundant but also didn't want to overcomplicate this. IDK thoughts?

[sjspielman]

I think the tests you've added are basically fine. Admittedly I am a test redundancy monster so I'm personally not going to complain about many tests especially since these are quick. That said, it's good enough here to just check column names and get rid of the expect_type() tests.

[sjspielman]

Two pretty quick comments, but then I think we're there :)

[sjspielman]

I think the tests you've added are basically fine. Admittedly I am a test redundancy monster so I'm personally not going to complain about many tests especially since these are quick. That said, it's good enough here to just check column names and get rid of the expect_type() tests.

[sjspielman]

I know I suggested this test structure, but just occurred to me that this would work and amount to the same thing:

Suggested change

	sweep_list_evaled |>
	purrr::map(
	\(df_evaled) {
	testthat::expect_named(
	df_evaled,
	c(
	"cell_id", "cluster", "algorithm", "weighting", "nn", "purity",
	"maximum_neighbor"
	)
	)
	}
	)
	})
	testthat::expect_named(
	purrr::list_rbind(sweep_list_evaled) ,
	c("cell_id", "cluster", "algorithm", "weighting", "nn", "purity", "maximum_neighbor")
	)

IMO this is more concise and readable? If you agree, let's just swap this in for these tests throughout this file!