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

Small refactorings to make code more pythonic #97

Closed
wants to merge 8 commits into from
Closed

Small refactorings to make code more pythonic #97

wants to merge 8 commits into from

Conversation

LucaCappelletti94
Copy link
Collaborator

No description provided.

@LucaCappelletti94
Copy link
Collaborator Author

I get a couple of errors from Travis which does not seem related to my commits, I think a couple of files are missing: TypeError: Could not find graph file data/ppismall/pos_train_edges and TypeError: Could not find graph file data/ppismall/pos_train_edges.

@callahantiff
Copy link
Collaborator

@LucaCappelletti94 - Sorry you are getting an error! This file has definitely had some significant changes made to it on the creating_keras_develop_branch branch, which has not yet been merged with other branches. Our thought being to make one giant PR from this branch when we had finished the full tf refactor. I'm now wondering if it might be worth it to make a branch of creating_keras_develop_branch and push some of the intermediate changes to files that are not dependent on the tf refactor.

What do you think about this? Sorry for the headache caused by these errors!

@justaddcoffee
Copy link
Member

@LucaCappelletti94 I think this should fix the problematic test here

@justaddcoffee justaddcoffee changed the base branch from master to develop March 12, 2020 18:41
@justaddcoffee
Copy link
Member

@LucaCappelletti94 I changed the base to develop (from master) - since generally we want to merge into develop

if there are no objections, it would also be helpful to make PRs from named branches (e.g. my_awesome_pr) on https://github.com/monarch-initiative/N2V instead of our individual forks of N2V (not strictly necessary though)

@callahantiff
Copy link
Collaborator

@justaddcoffee - what did you think about my comment above?

@justaddcoffee
Copy link
Member

@justaddcoffee - what did you think about my comment above?

Could we chat about how to do this? Possibly could just cherry-pick some commits from creating_keras_develop_branch into develop. Or possibly it might be simplest to just finish the tf refactor as soon as possible and then deal with all the differences between these two branches.

@callahantiff
Copy link
Collaborator

@justaddcoffee - what did you think about my comment above?

Could we chat about how to do this? Possibly could just cherry-pick some commits from creating_keras_develop_branch into develop. Or possibly it might be simplest to just finish the tf refactor as soon as possible and then deal with all the differences between these two branches.

Happy to do whatever is easiest, just thought it was worth mentioning. Either way, I'm happy to approve this PR once the merge conflicts are resolved.

Copy link
Collaborator

@callahantiff callahantiff left a comment

Choose a reason for hiding this comment

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

Sorry, I should have done this sooner. Looks fine to me (minus merge conflicts)!

@justaddcoffee
Copy link
Member

This is going to be hard to merge since it's based on master not develop - without any objection, I'm going to make a new PR for this and put Luca's nice improvements in that

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.

None yet

4 participants