search

bebop / ark

Go REST API to replace Genbank, Uniprot, Rhea, and CHEMBL
MIT License
23 stars 6 forks source link

Pathway query #23

Closed TimothyStiles closed 2 years ago

TimothyStiles commented 3 years ago

@jordan-strasser has created a sweet SQL query to generate new metabolic pathways. This PR is a work in progress and @Koeng101 is going to refactor the SQL into distinct queries after Jordan writes and confirms that his test case in pathways_test.go works as intended.

jordan-strasser commented 3 years ago

Didn’t intend to include that stuff, just didn’t know if it was safe to delete.

From: Tim @.***> Sent: Tuesday, September 14, 2021 12:20 PM To: allyourbasepair/allbase Cc: Jordan Wayne Strasser; Mention Subject: Re: [allyourbasepair/allbase] Pathway query (#23)

@TimothyStiles commented on this pull request.

Did you mean to include the swap and db-shm and db-wal files? This seems like accidental cruft to me.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/allyourbasepair/allbase/pull/23#pullrequestreview-754336252, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AUQJOWZUMKC6CZSBLNKDWU3UB6N5XANCNFSM5D3HJ6WQ. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

TimothyStiles commented 3 years ago

@jordan-strasser tests aren't passing and it looks like the one you wrote fails by default. Could you get the test to work before @Koeng101 takes a look at it?

TimothyStiles commented 3 years ago

Also it's safe to delete the cruft. Just delete and push your changes 🚀

jordan-strasser commented 3 years ago

This is strange, because all the tests I wrote passed on my computer. Not sure why it’s failing here

From: Tim @.> Sent: Tuesday, September 14, 2021 12:26:48 PM To: allyourbasepair/allbase @.> Cc: Jordan Wayne Strasser @.>; Mention @.> Subject: Re: [allyourbasepair/allbase] Pathway query (#23)

@jordan-strasserhttps://github.com/jordan-strasser tests aren't passing and it looks like the one you wrote fails by default. Could you get the test to work before @Koeng101https://github.com/Koeng101 takes a look at it?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/allyourbasepair/allbase/pull/23#issuecomment-919450089, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AUQJOW4CY75QQRMNUAZV4BLUB6OXRANCNFSM5D3HJ6WQ. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

Koeng101 commented 3 years ago

@jordan-strasser you can check why with "details"

For example:

pathways_test.go:11: Expected: len(result) = 9, Got:  0
--- FAIL: TestGetTotalPathways (0.00s)

Wonder why that is happening. Are the test files all there?

jordan-strasser commented 3 years ago

Oh the database isn’t there. Makes sense

From: Koeng101 @.***> Sent: Tuesday, September 14, 2021 12:39 PM To: allyourbasepair/allbase Cc: Jordan Wayne Strasser; Mention Subject: Re: [allyourbasepair/allbase] Pathway query (#23)

@jordan-strasserhttps://github.com/jordan-strasser you can check why with "details"

For example:

pathways_test.go:11: Expected: len(result) = 9, Got: 0--- FAIL: TestGetTotalPathways (0.00s)

Wonder why that is happening. Are the test files all there?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/allyourbasepair/allbase/pull/23#issuecomment-919457784, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AUQJOW5ZCHKKSSCENHE3LPDUB6QFPANCNFSM5D3HJ6WQ. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

Koeng101 commented 3 years ago

Uh oh. We didn't strip it down yet did we?

Koeng101 commented 3 years ago

(make sure to make backups before this) you can probably just run a query to delete the uniprot rows not associated with the coli genome.

jordan-strasser commented 3 years ago

Is that necessary? I think I just didn’t properly commit the database

From: Koeng101 @.> Sent: Tuesday, September 14, 2021 12:43:06 PM To: allyourbasepair/allbase @.> Cc: Jordan Wayne Strasser @.>; Mention @.> Subject: Re: [allyourbasepair/allbase] Pathway query (#23)

(make sure to make backups before this) you can probably just run a query to delete the uniprot rows not associated with the coli genome.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/allyourbasepair/allbase/pull/23#issuecomment-919460180, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AUQJOWY2P2HD3G7DSIZYDXTUB6QUVANCNFSM5D3HJ6WQ. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

Koeng101 commented 3 years ago

Not necessary strictly speaking, but I'd be willing to bet Tim will require for the merge. If you get it working, I can do the row dropping.

jordan-strasser commented 3 years ago

Does this test db need to be smaller to merge? I wanted to use it to test hybrid organisms too once we add more genbank files

From: Koeng101 @.***> Sent: Tuesday, September 14, 2021 12:45 PM To: allyourbasepair/allbase Cc: Jordan Wayne Strasser; Mention Subject: Re: [allyourbasepair/allbase] Pathway query (#23)

Not necessary strictly speaking, but I'd be willing to bet Tim will require for the merge. If you get it working, I can do the row dropping.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/allyourbasepair/allbase/pull/23#issuecomment-919461733, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AUQJOW3LUZ5PKKPG5TQB54LUB6Q6VANCNFSM5D3HJ6WQ. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

TimothyStiles commented 3 years ago

There's more to be done review wise but for now @jordan-strasser could you fix error handling in pathways.go?

TimothyStiles commented 3 years ago

@jordan-strasser I just ran my code coverage tool and it looks like you also needs tests to check for error handling in 'pathways.go'.

jordan-strasser commented 3 years ago

Does it give a line number? I just checked the code and it looks like all errors are checked for.

From: Tim @.> Sent: Thursday, September 16, 2021 11:33 AM To: allyourbasepair/allbase @.> Cc: Jordan Wayne Strasser @.>; Mention @.> Subject: Re: [allyourbasepair/allbase] Pathway query (#23)

@jordan-strasserhttps://github.com/jordan-strasser I just ran my code coverage tool and it looks like you also needs tests to check for error handling in 'pathways.go'.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/allyourbasepair/allbase/pull/23#issuecomment-921146284, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AUQJOW4VRLFIKU3E634BUITUCIZ73ANCNFSM5D3HJ6WQ. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

TimothyStiles commented 3 years ago

@jordan-strasser I did another round of review. Please make edits and ask as many questions as you need!

TimothyStiles commented 3 years ago

@jordan-strasser the test database is above ~10mb and you forgot to stop tracking your .swp file. Could you make the test database smaller and delete the .swp file?

jordan-strasser commented 3 years ago

I don't see the .swp file anywhere, and it's not possible for me to get the db down to 10mb without destroying the GetTotalPathways function. Just dropped 2 extraneous tables and got it from 44 mb to 40 mb

TimothyStiles commented 3 years ago

@jordan-strasser all tests and checks are passing but you still need to write tests to prove that error handling works correctly.

jordan-strasser commented 3 years ago

Do I need to specifically handle errors within the test functions? At each error in pathways.go I do log.Fatalf("...%d", err) to handle them internally so I can just have one thing returned