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

generalize correlation plugins #87

Merged
merged 9 commits into from
Mar 5, 2024
Merged

generalize correlation plugins #87

merged 9 commits into from
Mar 5, 2024

Conversation

d-callan
Copy link
Member

@d-callan d-callan changed the title correlation method now in utils rather than mbio R package generalize correlation plugins Feb 26, 2024
@d-callan d-callan marked this pull request as ready for review March 1, 2024 17:00
@d-callan d-callan requested a review from asizemore March 1, 2024 17:00
@d-callan
Copy link
Member Author

d-callan commented Mar 1, 2024

so this still feels a bit hacky to me, but its way better than it was. i dont have the prefilters working yet, but im thinking that can be a separate pr, bc i want to think about how to best do that yet.

also, i notice the values for member types are things like 'Host Eigengene' and i was expecting 'eigengene' regardless of host vs pathogen. i assume this is bc were also using these as display labels around places? if so, that makes me a bit uncomfortable that some fundamental logic here is based on completely free text strings as opposed to something w controlled values like at min an enum.

@asizemore
Copy link
Member

no the member should be 'eigengene'. I can start that change.

Copy link
Member

@asizemore asizemore left a comment

Choose a reason for hiding this comment

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

Had some questions but overall looks good. Are there parts that feel hacky to you outside the "member" values?

}

if (isEigengene) {
connection.voidEval("assayData <- assayData[order(" + computeEntityIdColName + ")]; " +
Copy link
Member

Choose a reason for hiding this comment

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

why order?

String statsCmd = "writeStatistics(computeResult, NULL, TRUE)";
if (isEigengene) {
connection.voidEval("abundanceData <- abundanceData[order(" + computeEntityIdColName + ")]; " +
"abundanceData <- abundanceData[, -as.character(" + dotNotatedIdColumnsString+ "), with=FALSE];" +
Copy link
Member

Choose a reason for hiding this comment

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

can we change this to 'assayData'? In the assaySelf version this naming distinction made it really clear what should be what.

boolean isEigengene = false;
if (collection1MemberType.contains("Eigengene") || collection2MemberType.contains("Eigengene")) {
if (!(collection1MemberType.contains("Eigengene") && collection2MemberType.contains("Eigengene"))) {
throw new IllegalArgumentException("Both collection variables must have the same member type.");
Copy link
Member

Choose a reason for hiding this comment

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

Is this just for now? Generally this statement is not true.

@asizemore
Copy link
Member

VEuPathDB/ApiCommonData@bee5e2c
Once JB patches the table we'll need to clear cache and will be good to go

@d-callan d-callan merged commit 3b18f7d into main Mar 5, 2024
@d-callan d-callan deleted the computes-refactor branch March 5, 2024 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants