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

feat: update all models #8

Merged
merged 13 commits into from
Jul 7, 2021
Merged

feat: update all models #8

merged 13 commits into from
Jul 7, 2021

Conversation

mihai-sysbio
Copy link
Member

This PR supersedes the other two PRs created with the same purpose, #4 an #7, which will be closed in favor of this one.

With this PR we fetch the latest (pre-)releases from all the models - model yml file and all existing annotation tsv files. The single exception is Yeast-GEM, where the latest release 8.5.0 changes the subsystems in a way that breaks compatibility with the maps. For this model, we will wait for the next release.

The file Yeast-GEM/subsystemSVG.tsv has been sorted alphabetically and the commented-out lines were removed.

From now on, all files in /integrated-models will be stored without Git LFS, in order to facilitate diff-ing and file content inspection.

This was referenced Jul 3, 2021
Copy link
Contributor

@inghylt inghylt left a comment

Choose a reason for hiding this comment

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

Things seems to work fine for Human and Yeast Gem. For the other models, gem-browser and interaction-partner random selection does not seem to be working correctly. Nothing is returned and the page just keeps on loading. And when trying to use the search bar, no/few matches are found. Some pages work but without data, e.g, http://localhost/explore/Mouse-GEM/gem-browser/subsystem/miscellaneous, while others have errors, e.g. http://localhost/explore/Worm-GEM/gem-browser/gene/M110.3 (I'm assume that gene ID:s are not updated)

@mihai-sysbio
Copy link
Member Author

mihai-sysbio commented Jul 7, 2021

Thank you @inghylt for noticing this. There is a version field in both the integratedModels.json and each model yml file. Since this is an eager integration of the updated models, the yml files have an empty version. Commit
902e0b8 addresses this. In the normal case of integrating updates, this should not be a problem.

Copy link
Contributor

@inghylt inghylt left a comment

Choose a reason for hiding this comment

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

Nice, from what I can tell it seems to work fine except the map ID:s which we will fix in upcoming issue. Left some suggestions to update the descriptions

Copy link
Contributor

@e0 e0 left a comment

Choose a reason for hiding this comment

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

I tested this using the feat/new-TSV-MA-908 branch from the neo4j-data-generation repo. Besides some unrelated issues (gene rule parsing) I think it looks good! Left a comment but it's not something that I think we should do in this PR even if we do want to address it.

@mihai-sysbio mihai-sysbio mentioned this pull request Jul 7, 2021
@@ -0,0 +1,2 @@
# compartment name must exist in the model
# filename must be a .svg with only [a-zA-Z0-9_] characters
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like the sentence reads better with filename must has an extension svg and with ...

Copy link
Contributor

@nanjiangshu nanjiangshu left a comment

Choose a reason for hiding this comment

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

The PR looks great! I have just left a few minor comments.

In addition, I have tested the deployment with the latest branch feat/new-TSV-MA-908 for the neo4j-data-generation repo, and it seems all features work as expected.
Especially for

  1. GeneRule with double brackets (( /explore/Human-GEM/gem-browser/reaction/MAR00080
  2. Coloring of gene boxes after setting Data Overlay on the 2D map viewer
  3. GEM comparison

@mihai-sysbio
Copy link
Member Author

Thank you all for the reviews. I hope the last commit isn't breaking things, so I'm going to merge.
I still think it would be nicer to have an improved solution to how the *SVG.tsv files are handled, but let's do that in #9 to keep the focus of this PR on updating the model files.

@mihai-sysbio
Copy link
Member Author

I could not find any SVG for Chondroitin sulfate degradation so the reference to it in Human-GEM as a subsystem SVG had to be removed in 811457d.

@mihai-sysbio mihai-sysbio merged commit b7a8957 into main Jul 7, 2021
@mihai-sysbio mihai-sysbio deleted the feat/update-all-models branch July 7, 2021 15:57
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.

4 participants