-
Notifications
You must be signed in to change notification settings - Fork 4
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
Feature/circular window #33
Conversation
bkm
commented
Jan 9, 2024
- import the files of metagenlab_lib code into zdb (in webapp.lib, the zdb folder was not intended to contain code)
- add support for circular contigs in the genomic region viewer
- several bugfixes introduced in format_pathway and format_module
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.
LGTM, just a few remarks:
- How do you get these "merge master" commits? I usually just rebase, then the history is clean (although I'm pretty sure it was a mess in this case, seing how much stuff I changed and how old some of your commits were).
- I think you could squash some of your commits together. I started reviewing commit by commit, but you actually go over the same things in several separate commits, making it very hard to review. In the end I basically gave up.
- I quickly tested your branch (ran the tests), and two things came up:
- We still need https://github.com/metagenlab/zDB/blob/master/webapp/manage.py#L5C1-L5C77 for testing purposes
- We need Make code compatible with biopython 1.8. metagenlab_libs#7 in order for things to work.
- You need to remove that line: https://github.com/metagenlab/zDB/blob/master/testing/webapp/test_views.py#L10
I've made these changes locally and could commit and push them to your branch if you want.
Also what is the plan on the metagenlab_libs side? Are we deleting from that repo the code we moved over?
And last thing. I get a whole bunch of warnings from pandas of the form
See the caveats in the documentation: https://pandas.pydata.org/pandas-docs/stable/user_guide/indexing.html#returning-a-view-versus-a-copy
df_seqids_circled.start_pos -= diff
/home/njohner/bin/zDB/webapp/chlamdb/views.py:1787: SettingWithCopyWarning:
A value is trying to be set on a copy of a slice from a DataFrame.
Try using .loc[row_indexer,col_indexer] = value instead
We should probably have a look.
Ah and we should discuss PEP8 😅 . I'll run another round of auto-PEP8 anyway, as the code from metagenlab_libs is not conform at all. But starting from then, I'd find it nice if new code that comes in conforms to the standards. It's pretty easy, you just need to install some plugin for whatever editor you use that will tell you if it's not.
I think I adressed most issues. Maybe have a look at the final diff : some issues you raised were already solved. Thanks for the remark about PEP8, I'll find a vim plugin. I used the git merge command. And indeed, the commits were a bit hard to follow, sorry about that. I need to get used to working with several people on the same project again. About metagenlab_libs, we need to check with Trestan. As for now, the repo is not needed for zdb anymore, so we could indeed delete the legacy code. As for the warnings, I did not have time to look into them yet. I'll do that the upcoming days. |
Had to uptade the code to reflect the changes in db_utils
Yes I saw that some remarks were for outdated code. I got used to reviewing on a per commit basis, but that only makes sense on a clean history. I didn't really want to look at the whole diff, because of the merge commits and because of moving the metagenlab_libs over. |