Skip to content

Submodule integration for piscem#50

Closed
jermp wants to merge 12 commits intodevfrom
submodule-integration-for-piscem
Closed

Submodule integration for piscem#50
jermp wants to merge 12 commits intodevfrom
submodule-integration-for-piscem

Conversation

@jermp
Copy link
Copy Markdown
Owner

@jermp jermp commented Aug 25, 2024

No description provided.

Copy link
Copy Markdown
Owner Author

@jermp jermp left a comment

Choose a reason for hiding this comment

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

Hi @rob-p and @NPSDC

I've incorporated the support for CUTTLEFISH input and the other things in the dev branch (see latest commit). The only thing missing is the alternative streaming query code.
Do you confirm the following
https://github.com/COMBINE-lab/piscem-cpp/blob/c5f1fe7cd7decb6012804ebef9dfe079502ac3b6/include/query/streaming_query_canonical_parsing.hpp
is the version I should look at?

@jermp jermp closed this Aug 30, 2024
@NPSDC
Copy link
Copy Markdown

NPSDC commented Aug 30, 2024

Hi @jermp .
Thank you so much for being so prompt in fixing the merge. The streaming query code is the same as the current sshash. Only thing to be added is the reset_state function.
https://github.com/COMBINE-lab/piscem-cpp/blob/c5f1fe7cd7decb6012804ebef9dfe079502ac3b6/include/query/streaming_query_canonical_parsing.hpp#L46

@jermp
Copy link
Copy Markdown
Owner Author

jermp commented Aug 31, 2024

@NPSDC @rob-p mmh...it's not the same as the version committed here in this branch
and in the submodule-integration-for-piscem, no?
It has some additional attributes and the additional function.
Also, the version we are looking at in piscem has a different logic for the lookup_advanced method.

Shall we also merge those changes?

@rob-p
Copy link
Copy Markdown
Contributor

rob-p commented Sep 2, 2024

Hi @jermp and @NPSDC,

So we are requesting the changes in the submodule-integration-for-piscem branch (i.e. that is the addition of the reset_state function and any new variables it touches).

However, what exists upstream in piscem now is needlessly complex, as it too tightly couples the streaming iterator and the k-mer iterator over the read. We have a new design that loosens the coupling by adding an adaptor to the streaming iterator to reset the state whenever the next k-mer queried is not at the position directly following the previous query. This achieves the same thing while not pushing too much extra complication into the streaming iterator in sshash upstream. Let me know if I've forgotten anything @NPSDC.

--Rob

@NPSDC
Copy link
Copy Markdown

NPSDC commented Sep 2, 2024

hi @jermp and @rob-p ,
So we are only requesting merge w.r.t sshash submodule-integration-for-piscem branch with the sshash dev branch. Nothing has to be done w.r.t piscem-cpp, since the piscem-cpp currently uses that sshash branch as a submodule, and accordingly its code has been updated (dev-atac). This branch for sshash adds the reset_state function in the streaming_canonical_query_file. Otherwise, this file has not been tampered with from the initial merge that was requested here
#47

@jermp
Copy link
Copy Markdown
Owner Author

jermp commented Sep 3, 2024

Hi @rob-p and @NPSDC, and thanks for confirming!
As of 9e48d6b, I've added the reset_state method to both the canonical and regular streaming query. (Actually, I plan to revisit that code/logic at some point.)

The dev branch now should be ready. Can you try to see if it works well for piscem?

Thanks!

@NPSDC
Copy link
Copy Markdown

NPSDC commented Sep 4, 2024

thanks @jermp . I am currently using the submodule-integration-for-piscem branch and doing some other testing. I will test the dev branch for piscem soon.

@jermp
Copy link
Copy Markdown
Owner Author

jermp commented Sep 5, 2024

@NPSDC, thank you very much. Let me know!

@jermp jermp deleted the submodule-integration-for-piscem branch May 23, 2025 13:59
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.

3 participants