-
Notifications
You must be signed in to change notification settings - Fork 0
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
Structure change #3
base: main
Are you sure you want to change the base?
Conversation
Can you remove the I recommend using a good global |
Removed cache files and added a .gitignore 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.
Nice work. I left a few comments to mainly update some of the code style a bit. Note that some of these comments re-occur in a few different places but I haven't marked all of them, so go through the code with them in mind.
Resolved all comments, updated the base algorithm of food counts computation. |
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 code is starting to look very nice, good job. I have one main concern, and that's the addition of several big(ish) sample files used for testing. This should ideally be avoided, see the comment in the code.
gfop/analysis.py
Outdated
import pandas as pd | ||
from sklearn.decomposition import PCA | ||
from sklearn.preprocessing import StandardScaler | ||
from skbio.stats.composition import clr |
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.
nitpick: Alphabetical order.
gfop/foodcounts.py
Outdated
""" | ||
Generates a table of food counts at the filename level. It filters the GNPS network | ||
based on the provided sample and | ||
reference groups. |
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.
nitpick: Newlines fix.
gfop/foodflows.py
Outdated
- flows: DataFrame with source, target, and value columns. | ||
- processes: DataFrame with process details. | ||
""" | ||
# Select GNPS job groups. |
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.
question: This first part repeats some behavior of the FoodCounts
class. Can FoodFlows
instead take a FoodCounts
object as input on which these steps are already applied, and then only take care of the visualization?
gfop/foodflows.py
Outdated
# Standard library imports | ||
import os | ||
import pkg_resources | ||
from importlib import resources |
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.
question: Not used?
gfop/visualization.py
Outdated
# Standard library imports | ||
import os | ||
from typing import List, Optional, Union, Tuple | ||
from importlib import resources |
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.
nitpick: Alphabetical order.
gfop/visualization.py
Outdated
|
||
# Third-party imports | ||
import matplotlib.pyplot as plt | ||
import matplotlib.figure |
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.
nitpick: Alphabetical order.
gfop/visualization.py
Outdated
# Third-party imports | ||
import matplotlib.pyplot as plt | ||
import matplotlib.figure | ||
import seaborn as sns |
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.
nitpick: Alphabetical order.
tests/test_foodcounts.py
Outdated
import sys | ||
|
||
# Third-party imports | ||
import pytest |
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.
nitpick: Alphabetical order.
tests/test_foodcounts.py
Outdated
# Internal imports | ||
from foodcounts import FoodCounts # type: ignore | ||
|
||
|
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.
nitpick: Too many blank lines.
Solved previous comments, and 8/11 issues. Issues remaining are to add changelog, documentation and contributing guidelines |
Changed structure of the package(added classes for food counts and food flows), added visualization, utilities and testing