Skip to content

Uploading Files and Tests of Metabolic Data#6

Open
VicTorres23 wants to merge 20 commits intomainfrom
UploadingMetabolicFilesAndTestFiles3rdTry
Open

Uploading Files and Tests of Metabolic Data#6
VicTorres23 wants to merge 20 commits intomainfrom
UploadingMetabolicFilesAndTestFiles3rdTry

Conversation

@VicTorres23
Copy link

Uploading 6 files related to the creation of the Exchange Metabolomics Data File used at AgileBioCyc to create a SmartTable and omics Dashboard analysis for each 5 organisms of the CarbStor project.

@mcnaughtonadm
Copy link
Contributor

@djinnome can this be merged?

@djinnome
Copy link
Contributor

djinnome commented Sep 6, 2024

@djinnome can this be merged?

I would like @tiekneeshrimp to go through it and clean up the code first. I don't think it will run as is.

from micom import load_pickle
from micom.viz import plot_tradeoff, plot_exchanges_per_sample, plot_growth

com1 = load_pickle("/Users/rodr579/Library/CloudStorage/OneDrive-PNNL/venv/CarbStor_Community/One.pickle")
Copy link
Contributor

Choose a reason for hiding this comment

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

Convert absolute path names to relative pathnames

# In[4]:


com2 = load_pickle("/Users/rodr579/Library/CloudStorage/OneDrive-PNNL/venv/CarbStor_Community_NoCurtobacterium/One.pickle")
Copy link
Contributor

Choose a reason for hiding this comment

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

Convert absolute pathnames to relative pathnames

@tiekneeshrimp
Copy link
Contributor

@djinnome can this be merged?

I would like @tiekneeshrimp to go through it and clean up the code first. I don't think it will run as is.

Thought this would be solved withhh issue 15? Maybe it can be merged?

Changed to relative (github) filepath
Copy link
Contributor

@mcnaughtonadm mcnaughtonadm Sep 9, 2024

Choose a reason for hiding this comment

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

These paths are close, but could benefit from slight tweaking. The way they are written, it could break if run on different systems. Let's add in a pathlib Path to make sure that everyone who runs this will be able to get the data.

First we should add:

from pathlib import Path

with the top imports.

Then we can set a few variables:

HERE = Path(__file__).parent.resolve()
ROOT = HERE.parent.resolve()
COM_MODEL = HERE.joinpath("One.pickle")
NO_CURTO = ROOT.joinpath("CarbStor_Community_NoCurtobacterium/One.pickle")

These will resolve to PosixPaths that we can leverage for the load_pickle methods.

com1 = load_pickle(COM_MODEL)
com2 = load_pickle(NO_CURTO)

this will make it so we don't have to guess at the folder structure of people using the code, it will just get there for us.

@tiekneeshrimp tiekneeshrimp self-assigned this Sep 13, 2024
Copy link
Contributor

@djinnome djinnome left a comment

Choose a reason for hiding this comment

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

Very nice!

from micom.viz import plot_tradeoff, plot_exchanges_per_sample, plot_growth

com1 = load_pickle("/Users/rodr579/Library/CloudStorage/OneDrive-PNNL/venv/CarbStor_Community/One.pickle")
com1 = load_pickle("/CarbStor_Community_Models/One.pickle")
Copy link
Contributor

Choose a reason for hiding this comment

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

When you put a / in front of the path, it makes it absolute. I think since the code is already in the CarbStor_Community_Models directory. you can just load_pickle("CarbStor_Community/One.pickle")

# In[4]:


com2 = load_pickle("CarbStor_Community_NoCurtobacterium/One.pickle")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should work.

@mcnaughtonadm
Copy link
Contributor

The last thing this needs @tiekneeshrimp is to remove a few things that are normally captured in a .gitignore file (for some reason that is missing in our repo, whoops)

There are 2 things that are typically not committed to the repo.

  1. .DS_Store :: an annoying file management hidden file created by Mac's Finder.
  2. __pycache__ :: a cache directory created when running python that has some of the cached compiled code.

Let's remove those for this PR. If you look in the Files Changed tab on this PR you can see where those things are located. I think the __pycache__ directory is located under the utils folder.

Copy link
Contributor

Choose a reason for hiding this comment

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

This entire directory should be removed. Just a rm -rf __pycache__ while in the utils directory

@tiekneeshrimp
Copy link
Contributor

@mcnaughtonadm resolved!

@mcnaughtonadm
Copy link
Contributor

Alright great, I pulled the branch and tried to run the code, there are a few things I noticed that are throwing errors and some formatting things that can be fixed to make this a full script!

I'll try to comment on each part of the file that needs some modification.

#!/usr/bin/env python
# coding: utf-8

# In[ ]:
Copy link
Contributor

Choose a reason for hiding this comment

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

This # In[ ]: should be removed throughout the script, they are artifacts from saving a jupyter notebook to python script and are just unnecessary clutter now.

Comment on lines +8 to +15
import pandas as pd
from cobra.io import read_sbml_model
from pathlib import Path

from micom import Community
from micom.workflows import build, grow, tradeoff, fix_medium,build_database
from micom import load_pickle
from micom.viz import plot_tradeoff, plot_exchanges_per_sample, plot_growth
Copy link
Contributor

Choose a reason for hiding this comment

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

We actually only need 2 of these import statements:

from pathlib import Path

from micom import load_pickle

the rest are unused in this script.

#set variables for importing data
HERE = Path(__file__).parent.resolve()
ROOT = HERE.parent.resolve()
COM_MODEL = HERE.joinpath("One.pickle")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs an additional folder:

COM_MODEL = HERE.joinpath("CarbStor_Community/One.pickle"

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I think I messed you up with giving the ROOT, I wasn't sure where that file was being run from. Try something like this:

# set variables for importing data
HERE = Path(__file__).parent.resolve()
COM_MODEL = HERE.joinpath("Carbstor_Community/One.pickle")
NO_CURTO = HERE.joinpath("CarbStor_Community_NoCurtobacterium/One.pickle")

Comment on lines +24 to +39
#load communities and define parameters
com1 = load_pickle(COM_MODEL)
result1 = com1.optimize(fluxes=True, pfba=True)
#result1
result1.summary()


# In[4]:

com2 = load_pickle(NO_CURTO)
result2 = com2.optimize(fluxes=True, pfba=True)
#result2
result2.summary()


# In[ ]:
Copy link
Contributor

@mcnaughtonadm mcnaughtonadm Sep 13, 2024

Choose a reason for hiding this comment

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

2 things here. First let's wrap this code into a main() method:

def main():
     com1 = load_pickle(COM_MODEL)
     ... the rest of the code on same indentation level ...
     result2.summary()

then adding at the bottom of the file:

if __name__ == "__main__":
     main()

Second thing is that result1.summary() and result2.summary() don't actually work outside of a jupyter notebook with micom for some weird cobrapy integration reason. Therefore we need to do something else here as an output to this script.

One thing I think we can do is return the community members. This can be done by replacing those summary() lines with print(result1.members) and the same for result2.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we do the same pathlib treatment for the paths present in this file?

Copy link
Contributor

@djinnome djinnome left a comment

Choose a reason for hiding this comment

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

looks good to me. Tests pass according to @tiekneeshrimp

@djinnome
Copy link
Contributor

djinnome commented Sep 17, 2024

@tiekneeshrimp will move directories:

  • move source code in concerto to src/concerto
  • Move test code to the tests directory and confirm the tests still pass
  • Modify the test code if they don't pass

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.

4 participants