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

correlation volcano #2620

Merged
merged 24 commits into from
Jan 18, 2025
Merged

correlation volcano #2620

merged 24 commits into from
Jan 18, 2025

Conversation

xzhou82
Copy link
Collaborator

@xzhou82 xzhou82 commented Jan 17, 2025

Description

Checklist

Check each task that has been performed or verified to be not applicable.

  • Tests: added and/or passed unit and integration tests, or N/A
  • Todos: commented or documented, or N/A
  • Notable Changes: updated release.txt, prefixed a commit message with "fix:" or "feat:", added to an internal tracking document, or N/A

@xzhou82 xzhou82 marked this pull request as draft January 17, 2025 13:39
@xzhou82 xzhou82 linked an issue Jan 17, 2025 that may be closed by this pull request
@xzhou82 xzhou82 requested a review from creilly8 January 17, 2025 16:07
termfilter: any
termdbConfig: {
correlationVolcano: any
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this should be optional

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. This is fixed.

names(output)[3] <- "original_p_value"
names(output)[4] <- "adjusted_p_value"
names(output)[5] <- "sample_size"
cat(paste0("adjusted_p_values:", toJSON(output)))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

change it to just return stringnified json, rather that nodejs has to cut a line and check what it is

Copy link
Contributor

Choose a reason for hiding this comment

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

This makes it easier to print variables inside the R script for debugging. If this is not added, nodejs considers the entire output (including other print statements from R) as JSON which breaks PP during debugging. Most of my other R and rust scripts are designed this way so its easier to debug R and rust codes.

Copy link
Collaborator Author

@xzhou82 xzhou82 Jan 18, 2025

Choose a reason for hiding this comment

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

either use message(msg) to print to stderr (don't do if this throws in node)

or better and i highly prefer this, you should have a test script to run the r script with test data on commandline, that will be more efficient than having to refresh the web browser each time, and be able for script to cleanly return json

@xzhou82 xzhou82 marked this pull request as ready for review January 17, 2025 22:28
server/utils/corr.R Outdated Show resolved Hide resolved
@xzhou82 xzhou82 merged commit 48b67e4 into master Jan 18, 2025
2 of 3 checks passed
@xzhou82 xzhou82 deleted the issue.2619 branch January 18, 2025 02:18
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.

correlation volcano initial prototype
3 participants