Skip to content

Conversation

@agitter
Copy link
Collaborator

@agitter agitter commented Apr 20, 2023

Work in progress pull request for #76. I'll edit this to add details when it's ready for review.

agitter added 9 commits April 20, 2023 10:02
Refactor as needed
Rename Dataset.py to lower case
It was making the module import due to the name collision
No longer needed now that SPRAS uses Docker for
pathway reconstruction algorithms
Good ideas in the prototype but current design
has diverged considerably.
No immediate plans to add BowTieBuilder
@agitter
Copy link
Collaborator Author

agitter commented Apr 21, 2023

The scope of this pull request is large enough so I'm going to stop here. This closes #76 even though it does not add pre-commit hooks. Reviewing the commit messages is the best way to get an overview of the changes made. To summarize:

  • Closes proposed change to src code organization #7 by moving all *.py files out of the top level of the repository into appropriate subdirectories and removing an unnecessary level of analysis subdirectories
  • Following Python file naming conventions for *.py files and refactoring code accordingly
  • Closes candidate files to remove #6 by deleting the old prototype files. They are interesting relics of old design ideas, but they will confuse new contributors and clutter the repository. Also removes the docker-demo for the same reason and should also close Clean up non-functional components #29.
  • Closes Switch pathway output format to tab-separated text #75 by switching the universal output pathway file format to tab-separated and adding an example of nodes with spaces in the names for the ML analysis testing.
  • Other code factoring to address Python linting issues.

@ntalluri can you please review these changes? Some affect your ML code, and you also have a good sense of how all of the SPRAS components work together.

@annaritz you don't need to do a full review but could quickly browse the summary above and commit messages to make sure you don't object to any changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I had to update the test files to work with the version 2 code so directory names are different to make it work with the updated ml code

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can manually tab separate these files once these changes are added

Copy link
Collaborator

@ntalluri ntalluri Apr 26, 2023

Choose a reason for hiding this comment

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

can keep this, I can just update the names to work with the new code that is used in version 2

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can update the tab seperated part on my end since I had to change names that are in this file for version2

@annaritz
Copy link
Contributor

This all looks great to me! Thanks @agitter and @ntalluri for tying up these loose ends.

@agitter
Copy link
Collaborator Author

agitter commented Apr 27, 2023

@ntalluri and I discussed her review comments during our meeting today. These changes will create conflicts with her work in #80. Our plan is to merge this during our next meeting and clean up the merge conflicts together.

@agitter agitter merged commit ea5b5fd into Reed-CompBio:master May 3, 2023
@agitter agitter deleted the refactoring branch May 3, 2023 21:12
agitter added a commit that referenced this pull request May 4, 2023
Match behavior added in #79
Lyce24 pushed a commit to Lyce24/spras that referenced this pull request Jun 9, 2023
This reverts commit ea5b5fd, reversing
changes made to 5334cd4.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants