-
Notifications
You must be signed in to change notification settings - Fork 14
Add new method for FBA analysis #349
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
base: master
Are you sure you want to change the base?
Conversation
🔍 Vulnerabilities of
|
digest | sha256:1da4592f2023b41f99f5b297fe70a1a045827ae3517e1a8d4b45f493cdee0c86 |
vulnerabilities | |
platform | linux/amd64 |
size | 808 MB |
packages | 427 |
📦 Base Image oisupport/staging-amd64:12-slim
also known as |
|
digest | sha256:364d3f277f79b11fafee2f44e8198054486583d3392e2472eb656d5c780156f5 |
vulnerabilities |
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
Description
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
Description
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
Description
Description
Description
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
|
Recommended fixes for image
|
Digest | |
Vulnerabilities | |
Size | 0 B |
Packages | 0 |
Refresh base image
Rebuild the image using a newer base image version. Updating this may result in breaking changes.✅ This image version is up to date.
Change base image
✅ There are no tag recommendations at this time.
Update with the merge from Master.
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.
I haven't looked very deeply at most of these scripts yet, but I figured I might as well post the feedback I currently have as it'll completely change how this PR looks. The biggest issue right now is the amount of duplicated code. You could tackle this by creating the helper function I outlined in one of my comments and import it into the relevant modules.
target_variants = params.get("variant", None) | ||
target_generation = params.get("generation", None) | ||
|
||
# Filter by specified variants and generations |
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.
You could add this filter to the SQL query to speed that up (potentially by a lot if only reading a small subset of a large workflow output) and reduce RAM usage.
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.
This comment still applies. I think keeping RAM usage down is particularly important for multivariant/multiseed analyses where the amount of data being read can potentially be insanely high.
Also, to solve the import issue with Escher in the Pytest, you'll need to add it to the package list with uv. I think How big is the Escher package and its dependencies BTW? You can get this by comparing Can you check the size of plotly and ipykernel as well? |
Merge with up-to-date Master.
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.
I only skimmed most of this, because there's just too much to review in depth. The biggest area for improvement right now is redundancy across a lot of scripts. You have, for the most part, already packaged a lot of duplicated code into helper functions, so refactoring should hopefully not be too hard. The type errors in Mypy might need explicit type hints to fix, and my previous comment about adding Escher with uv still applies (to fix the failing pytests).
# Filter by specified generations if provided | ||
if target_generations is not None: | ||
print(f"[INFO] Target generations: {target_generations}") | ||
df = df.filter(pl.col("generation").is_in(target_generations)) |
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.
You could build this generation filter into your SQL query with a WHERE
clause to be more efficient.
SELECT time, generation, | ||
counts as protein_count | ||
FROM unnested_counts | ||
WHERE idx = {protein_idx + 1} -- SQL uses 1-based indexing |
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.
This is overkill. I'd recommend SELECT listeners__monomer_counts[{protein_idx + 1}] AS cnt
or using ecoli.library.parquet_emitter.named_idx
.
) | ||
gen_summary_pd["lower"] = ( | ||
gen_summary_pd["mean_count"] - gen_summary_pd["std_count"] | ||
) |
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.
Do you use the upper
and lower
columns?
|
||
# Filter by specified variants and generations if provided | ||
target_variants = params.get("variant", None) | ||
target_generations = params.get("generation", None) |
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.
Highly recommend building these filters into the SQL query. For a multivariant analysis, the time saved by not reading some of the data can become significant.
avg_catalyst_counts[biocyc_id] = variant_avgs | ||
|
||
# Create visualization based on layout mode | ||
if layout_mode == "grid": |
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.
Consider moving the code in this if...else
into the above one for continuity.
@@ -0,0 +1,834 @@ | |||
""" | |||
This script preprocesses FBA flux data by mapping extended reactions to base reactions, |
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.
There's lots of redundant code across the multigeneration fba_flux_
analyses. You've already organized things into helper functions so it may not be too hard to fix this.
@@ -0,0 +1,1004 @@ | |||
""" | |||
This script preprocesses FBA flux data by mapping extended reactions to base reactions, |
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.
Also a lot of redundant code.
@@ -0,0 +1,380 @@ | |||
""" | |||
Visualize FBA reaction flux dynamics using PCA trajectory analysis. |
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.
Interesting idea. Do you have any examples of reaction IDs that yielded interesting plots?
target_variants = params.get("variant", None) | ||
target_generation = params.get("generation", None) | ||
|
||
# Filter by specified variants and generations |
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.
This comment still applies. I think keeping RAM usage down is particularly important for multivariant/multiseed analyses where the amount of data being read can potentially be insanely high.
Add
cell growth rate
method for multivariant analysis, and modifycell mass
method.Both are compatible for multigeneration analysis, and can set
variant
andgeneration
params to specify the variant and generation ID you wanna to analysis.