Skip to content

Ampliconclip report#2039

Merged
whitwham merged 2 commits into
samtools:developfrom
bwlang:ampliconclip_report
May 16, 2024
Merged

Ampliconclip report#2039
whitwham merged 2 commits into
samtools:developfrom
bwlang:ampliconclip_report

Conversation

@bwlang

@bwlang bwlang commented Apr 24, 2024

Copy link
Copy Markdown
Contributor

fixes #2033

@bwlang

bwlang commented Apr 24, 2024

Copy link
Copy Markdown
Contributor Author

Seems like there is something up with github's checking for mergability being too slow for appveyor to know that it can run. Is it possible to manually trigger the appveyor test? It did succeed for an earier commit (348619e)

@bwlang

bwlang commented Apr 24, 2024

Copy link
Copy Markdown
Contributor Author

@whitwham : i think this is ready for review now

@whitwham

whitwham commented Apr 24, 2024

Copy link
Copy Markdown
Member

@bwlang I'll take a look next week when I'm back in the office. Thanks.

@bwlang

bwlang commented Apr 27, 2024

Copy link
Copy Markdown
Contributor Author

looks like the ubuntu build is failing due to configure's c compiler test.

Found candidate GCC installation: /usr/bin/../lib/gcc/x86_64-linux-gnu/13
Selected GCC installation: /usr/bin/../lib/gcc/x86_64-linux-gnu/13
Candidate multilib: .;@m64
Selected multilib: .;@m64
configure:3131: $? = 0
configure:3120: clang -V >&5
clang: error: argument to '-V' is missing (expected 1 value)
clang: error: no input files
configure:3131: $? = 1
configure:3120: clang -qversion >&5
clang: error: unknown argument '-qversion'; did you mean '--version'?
clang: error: no input files
configure:3131: $? = 1
configure:3120: clang -version >&5
clang: error: unknown argument '-version'; did you mean '--version'?
clang: error: no input files
configure:3131: $? = 1
configure:3151: checking whether the C compiler works
configure:3173: clang -g -Wall -O3 -fsanitize=address  -fsanitize=address -Wl,-rpath,`pwd`/inst/lib conftest.c  >&5
/usr/bin/ld: cannot find /usr/lib/llvm-18/lib/clang/18/lib/linux/libclang_rt.asan_static-x86_64.a: No such file or directory
/usr/bin/ld: cannot find /usr/lib/llvm-18/lib/clang/18/lib/linux/libclang_rt.asan-x86_64.a: No such file or directory
clang: error: linker command failed with exit code 1 (use -v to see invocation)

I think that's unrelated to this code.

@jkbonfield

jkbonfield commented Apr 29, 2024

Copy link
Copy Markdown
Contributor

Thanks. I'm testing a cirrus-ci change to fix this in htslib and if that cures it I'll migrate it over to the other packages too.

It looks like Ubuntu broke their latest clang build. :(

@bwlang

bwlang commented May 5, 2024

Copy link
Copy Markdown
Contributor Author

looks like htslib is still failing, but in a different way now

@whitwham

whitwham commented May 7, 2024

Copy link
Copy Markdown
Member

Hi @bwlang, let me know when you are ready for me to review and merge this PR. Thanks.

@bwlang

bwlang commented May 7, 2024

Copy link
Copy Markdown
Contributor Author

@whitwham I think it's ready for your feedback now. Hopefully the htslib issue we see from the build systems will get resolved soon too.

whitwham
whitwham reviewed May 9, 2024
View reviewed changes
Comment thread bam_ampliconclip.c Outdated
for (int i = 0; i < sites->length; i++) {
fprintf(bed_count_summary_fp, "%s\t%"PRId64"\t%"PRId64"\t%s\t%s\t%s\t%"PRId64"\n",
kh_key(bed_hash, itr), sites->bp[i].left, sites->bp[i].right, sites->bp[i].name,
sites->bp[i].score, (sites->bp[i].rev ? "-" : "+") , sites->bp[i].num_reads);

@whitwham whitwham May 9, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

bp[i].rev can be uninitialised if the --strand option is not used.

@bwlang bwlang May 11, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've added this, but i think it's a bit strange that we modify the bed information... though this does accurately describe the count (which is not stranded now)

 char* strand_out = param->use_strand ? (sites->bp[i].rev ? "-" : "+") : ".";

I could change the struct to store a char instead of rev (more invasive, but maybe nicer than an int ?)

@whitwham whitwham May 13, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The rev in the struct is used by ampliconstats as well as ampliconclip so I would prefer not to change it.

You could add another member to the structure to store the strand symbol (if present).

@bwlang bwlang May 13, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Do you think it's better to

  1. store the char in the struct (which could in theory be out of sync with rev)
    or
  2. recalc like this and switch the strand column to '.' indicating that the count is unstranded ?
    (i implemented # 2, but not sure if that's best).

@whitwham whitwham May 13, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm, leave it the second way for now. I can always change it later if need be.

@whitwham

whitwham commented May 9, 2024

Copy link
Copy Markdown
Member

Good, apart from the comment I added.

If you could squash the commits down to a more reasonable number that would be great.

@whitwham

whitwham commented May 9, 2024

Copy link
Copy Markdown
Member

Also, rebasing should help with the failed test.

@bwlang

bwlang commented May 11, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

looks like it still can't configure htslib.
I can force push a squashed version, or maybe it's easy to squash while merging?

@bwlang bwlang force-pushed the ampliconclip_report branch from ef3ae41 to 7f393fa Compare May 14, 2024 18:01
@bwlang

bwlang commented May 15, 2024

Copy link
Copy Markdown
Contributor Author

@whitwham
good to go?

@whitwham

whitwham commented May 16, 2024

Copy link
Copy Markdown
Member

@bwlang Yes, but I have been distracted by non samtools work. I should have time to look at it today.

@whitwham whitwham force-pushed the ampliconclip_report branch from 7f393fa to a6b9501 Compare May 16, 2024 09:10
@whitwham whitwham merged commit d1c40a4 into samtools:develop May 16, 2024
@whitwham

whitwham commented May 16, 2024

Copy link
Copy Markdown
Member

Thank you. I rebased and force pushed to make the tests work again.

@bwlang

bwlang commented Aug 6, 2024

Copy link
Copy Markdown
Contributor Author

@daviesrob
I'd love to get this into production, but that's not easy for us without a release. Seems like a bunch of stuff has been done since April... is there one planned soon? Anything the community can help with to make it easier?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@whitwham whitwham whitwham left review comments

Assignees

@whitwham whitwham

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add a report of number of alignments for each primer to samtools ampliconclip

3 participants

@bwlang @whitwham @jkbonfield