-
-
Notifications
You must be signed in to change notification settings - Fork 29
Oncotree migration #318
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
base: feature/oncotree-migration
Are you sure you want to change the base?
Oncotree migration #318
Conversation
web/src/main/go/package-lock.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should delete this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete the file or gitignore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The package-lock.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is generated automatically by the annotations you added to the files in server/main.go. Could you add documentation on how to regenerate? Also, will regenerating recreate the json and yaml versions as well?
NIT: Maybe we can put the generated files in swagger/generated. That way, it's obvious these are the swagger docs and that it's generated so people don't mess with the files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add the dependencies in the go.mod at the root of the go directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as go.mod
web/src/main/go/cmd/server/main.go
Outdated
| gin.DefaultErrorWriter = log.WriterLevel(logrus.ErrorLevel) | ||
|
|
||
| // TODO: can we configure the static folder location? | ||
| router.Static("/assets", "../resources/static/assets") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
router.Static("/", "../resources/static") doesn't work. It doesn't read /assets
| return | ||
| } | ||
|
|
||
| flattened := flattenTumorTypes(tree) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably no need to flatten the tree first. Can't we just search the tumor types in a BFS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented
| Visible bool `json:"visible"` | ||
| } | ||
|
|
||
| func getHardcodedVersions() []Version { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with hardcoding the identifiers, but we probably shouldn't hardcode the release date. Since they change, we would need to update the code once we change them. We should set up the system to use the most recent tree as latest stable, and we need to find a way to denote candidate.
Need to make sure to communicate this change with Ritika
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented.
- The version identifier remains consistent, but the release date is refreshed to reflect the most recent release.
- Whenever a new input version is detected, the system resolves and validates it by locating the latest file and updating the version reference accordingly.
web/src/main/go/cmd/server/utils.go
Outdated
|
|
||
| func isValidVersion(version string) bool { | ||
| treeDir := "../../../../trees" | ||
| versions, err := GetAvailableVersionIdentifiers(treeDir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would probably be better practice to just look for the file we are interested in instead of getting all available versions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, changed
web/src/main/go/cmd/server/utils.go
Outdated
| return node.Code | ||
| case "name": | ||
| return node.Name | ||
| case "maintype": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the current API uses mainType with capital T
| for _, lvlStr := range strings.Split(levelsStr, ",") { | ||
| lvlStr = strings.TrimSpace(lvlStr) | ||
| lvl, err := strconv.ParseUint(lvlStr, 10, 32) | ||
| if err == nil && lvl != 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we handle error by returning BadRequest instead of skipping over them?
web/src/main/go/cmd/server/utils.go
Outdated
| return max | ||
| } | ||
|
|
||
| func writeTSVRows(node internal.TreeNode, levels []string, sb *strings.Builder) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you just get the sorted nodes (you could figure out the max level during this process) and then write the rows in a for loop. The recursive logic where you pass in an empty string array as well as the string builder is a bit confusing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented
* add tsv parsing step * add precursors automatically from ampping * populate precursors and revocations from last available tree with no mapping * add tree generation to git workflow
* fix docker build and add tree files * remove test trees * remove print statements * remove static files from source control
No description provided.