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

Make GNN plots available as an SPT CLI command and API endpoint #320

Merged
merged 19 commits into from May 15, 2024

Conversation

CarlinLiao
Copy link
Collaborator

To close #319.

@CarlinLiao CarlinLiao added the feature New feature label May 9, 2024
@CarlinLiao CarlinLiao self-assigned this May 9, 2024
@CarlinLiao
Copy link
Collaborator Author

Although I wanted to change the database query to be direct instead of through the API server, for expediency's sake + don't-fix-what-ain't-broke, I'll stick with the API server counts implementation and commit a thinner version of DataAccessor for now.

@CarlinLiao
Copy link
Collaborator Author

test/apiserver/unit_tests/record_counts1.txt and build/build_scripts/expected_table_counts.txt are identical. Is this redundancy necessary?

@jimmymathews
Copy link
Collaborator

This work entails updating the documentation in spt-data:
https://github.mskcc.org/mathewj2/spt-data/tree/main/findings

Also, I suppose, committing the plot specification artifacts to spt-data for now. Similar to findings.

@CarlinLiao CarlinLiao marked this pull request as ready for review May 15, 2024 16:04
@CarlinLiao
Copy link
Collaborator Author

All tests pass except for workflow tests, which complain about default_study_lookup not being present. Not sure if it's unrelated or if the change to the first test database broke something.

@jimmymathews
Copy link
Collaborator

I would check that the new system of data-preloaded-images is getting used (in a recent update I moved these images to docker hub, so development doesn't require building them locally).

@jimmymathews
Copy link
Collaborator

test/apiserver/unit_tests/record_counts1.txt and build/build_scripts/expected_table_counts.txt are identical. Is this redundancy necessary?

We will have to check usage, probably the idea was that the test and build scripts would not need to reach into each other's directory contents.

@CarlinLiao
Copy link
Collaborator Author

If the data loaded images live remotely, and I've modified one of the data loaded images to have two importance score studies so the plotting has something to compare, would that cause an issue?

@jimmymathews
Copy link
Collaborator

Yes, this is the scenario where, when this PR is complete, we would update the docker hub images.
I suppose for now you would need to make sure that your development setup is using your locally-built images. I'm not sure what docker does when both remote and local are available, I think it depends on the pull policy.

Comment on lines +13 to +16
spt graphs upload-importances --config_path=build/build_scripts/.graph_transformer.config --importances_csv_path=test/test_data/gnn_importances/1.csv

spt db upload-sync-findings --database-config-file=build/db/.spt_db.config.local test/test_data/findings.json
spt db upload-sync-small --database-config-file=build/db/.spt_db.config.local findings test/test_data/findings.json
spt db upload-sync-small --database-config-file=build/db/.spt_db.config.local gnn_plot_configurations test/test_data/gnn_plot.json
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Both old and new versions of this build script reach into the test directory for data.

@jimmymathews
Copy link
Collaborator

Tests passed.

@jimmymathews jimmymathews merged commit f1ce820 into main May 15, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make GNN plots available as an SPT CLI command and API endpoint
2 participants