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

Initial edge updates #76

Merged
merged 22 commits into from
Nov 7, 2024
Merged

Initial edge updates #76

merged 22 commits into from
Nov 7, 2024

Conversation

dkhofer
Copy link
Collaborator

@dkhofer dkhofer commented Oct 25, 2024

No description provided.

src/main.rs Outdated Show resolved Hide resolved
@@ -1,4 +1,3 @@
use crate::models::block_group::BlockGroup;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just misc warning cleanup here and a few other places. It's work to pick through them to find legit compilation errors

@@ -801,6 +802,41 @@ pub fn move_to(conn: &Connection, operation_conn: &Connection, operation: &Opera
}
}

pub fn start_operation<'a>(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just some refactoring

@dkhofer dkhofer requested a review from Chris7 October 29, 2024 17:54
@dkhofer dkhofer force-pushed the initial-edge-updates branch 2 times, most recently from 347f7fd to df47be7 Compare October 31, 2024 14:16
@@ -146,6 +147,25 @@ impl Path {
rows.next().unwrap().unwrap()
}

pub fn full_query(conn: &Connection, query: &str, params: impl Params) -> Vec<Path> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the difference here and other queries? Does this version support carray?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It does, but it turns out I don't need that for this PR. I'll put together a separate PR with this change for the places that need it

}
}

let new_name = format!(
Copy link
Collaborator

Choose a reason for hiding this comment

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

i was thinking the fasta header should be part of the new name. So it would be like chr1-GFP, etc.

};
use crate::{calculate_hash, operation_management};

pub fn update_with_fasta(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this compatible as a branch off of #79 ? Where insert_bg_changes is the way to give updates? We're quickly moving to the multiple engineering round setups where this is still limited to 1 round. (e.g. we do one round of changes to insert a landing pad, and then another round to insert many genes into it. We can't refer to the landing pad region here).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let's say some fasta was imported by operation 1, and the region we want to replace with a landing pad is 10-20 on chr1.

gen update --fasta landing-pad.fa --start 10 --end 20 --contig-name chr1

Let's say that gets an operation ID of 2. This creates a new path for chr1 and adds the non-chr1 paths from operation 1, plus the new chr1, as entries in operation_paths associated with this operation 2. That means the current chr1 path (with landing pad) is what the next operation will reference for coordinates.

Let's say we want to integrate a gene to the landing pad starting at 12, going to 100. (No idea how biologically valid that is.)

gen update --fasta landing-pad.fa --start 12 --end 100 --contig-name chr1

That creates another new path, replacing the one that was created for operation 2. All the non-chr1 paths stay the same from operation 1.

Does that help explain my thinking?

Copy link
Collaborator

@Chris7 Chris7 left a comment

Choose a reason for hiding this comment

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

Just a few comments worth checking on. No need to send back if things are ok.

Some(parent_sample_name),
)
.unwrap();
BlockGroup::clone(conn, block_group.id, new_bg_id);
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's the reason for the second clone?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, I didn't see that get_or_create_sample_block_group did the clone. Removed this line, thanks!

panic!("No block group found with region name: {}", region_name);
}

let path = BlockGroup::get_current_path(conn, new_block_group_id);
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should ensure the clone in blockgroup orders the newly created paths by order of id too so this actually returns the first path made in a cloned bg.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just made that change

@@ -243,7 +244,7 @@ pub fn update_with_vcf<'a>(
collection_name,
&fixed_sample,
seq_name.clone(),
coordinate_frame,
Some(""),
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's the reason for these? it should already be passing None

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Long story short, I was confused about our sample name conventions. I've reverted most of the changes to this file

@dkhofer dkhofer merged commit 0a8b366 into main Nov 7, 2024
1 check passed
@dkhofer dkhofer deleted the initial-edge-updates branch November 7, 2024 22:04
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.

2 participants