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

Msgrowthphenotypes #21

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

freiburgermsu
Copy link

Hello @cshenry !

Here are edits and error corrections of MSGrowthPhenotypes:

  1. an unused import is flagged
    
  2. variable definitions are consolidated
    
  3. redundant Nones in conditionals are removed
    
  4. a missing self. is added to the parents variable, which prevents a NameError for the undefined variable.
    
  5. default initial values replace an else clause
    
  6. PEP-8 spacing is introduced to function parameters
    
  7. list comprehension consolidates and expedites a few loops
    
  8. a missing base_media argument in the from_kbase_file function is added to the end of the function, which conserves backcompatibility while preventing a NameError from the undefined variable
    
  9. read_table is leveraged to construct a DataFrame the KBase TSV file, instead of manually parsing the entire file
    
  10. The passage of headings to FBAHelper.validate_dictionary would cause an error, where the former was a list yet the latter requires a dictionary. This is resolved by passing row.to_dict which provides a key:value structure to [each row of a DataFrame](https://pandas.pydata.org/pandas-docs/stable/reference/api/pandas.Series.to_dict.html).
    
  11. DataFrames do not possess a rows attribute, hence this error is circumvented with the supported iterrows().
    
  12. content is added to the DataFrame a row at a time, instead of constructing a dictionary of lists and then creating a DataFrame from it at the end
    

Thank you for your review :)
Andrew

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.

1 participant