Skip to content

Conversation

@e-osipov
Copy link

@e-osipov e-osipov commented Jun 4, 2025

Findseq worked incorrectly when haystack spans multiple objects. In this case, the same residue IDs selected in all haystack objects.

Proposed change iterates over all objects in haystack and searches needle in each of them independently and finally combines them into the single search object.

@pslacerda
Copy link
Member

Hi, thank you! Can you resolve this branch conflict before we test it?

@e-osipov
Copy link
Author

e-osipov commented Jun 6, 2025

Hi!

This is my first commit so I am not sure which conflict should be resolved. I see that the changes could be merged cleanly:
image
Could you point to the source of conflict?

Thanks!

@pslacerda
Copy link
Member

I'm seeing this image

I'll analyze better from 16/Jun forwards.

@pslacerda
Copy link
Member

@JarrettSJohnson and @speleo3, what do you see?

@speleo3
Copy link
Contributor

speleo3 commented Jun 19, 2025

I suggest to make a pull request with the findseq branch instead of e-osipov:master. Then rebasing should be possible or at least the merge conflicts would become obvious.

image

On a different note: What's the status of the other findseq pull request from @jrom99? Those two would have massive merge conflicts, so would be good to clarify which one has priority. Maybe the multi-object logic can be added to #152 as well.

@pslacerda
Copy link
Member

pslacerda commented Jun 19, 2025

#152 says the following in a comment:

Another issue I've noticed is that for objects which have residue sequence data available, but the residues don't have structural information (like in loops), this script is unable to find the sequence to select..

Would be awesome if you can fix #152, I tested it and couldn't make it work and both issues seems valid to me.

@jrom99
Copy link
Contributor

jrom99 commented Jun 19, 2025

On a different note: What's the status of the other findseq pull request from @jrom99? Those two would have massive merge conflicts, so would be good to clarify which one has priority. Maybe the multi-object logic can be added to #152 as well.

Sorry, I didn't test yet what was the problem with my pull request. Since it's working as expected with all the files that I work with, it is possible that my data is some kind of edge case.

That said, I have some suggestions for the code:

  • use cmd.get_unused_name(prefix) instead of using `"foundSeq" + str(random.randint(0, 32000))
  • try the oneletter cmd.iterate variable instead of the one_letter dictionary.
  • move the checkParams call to outside the cmd.get_object_list loop
  • use cmd.get_unused_name instead of using a __h haystack selection

The firstOnly behavior in this case is the first match per object, is it the correct action?

Another thing is that some files may have multiple segi but the same chain name (see https://pymolwiki.org/index.php/Selection_Macros), I didn't get the chance to test cmd.get_fasta_string to see if it would solve this problem, but it would be interesting to take it into consideration.

Edit: @pslacerda can you list the tests that you did and files that you used for them? I'll try testing over the weekend

@pslacerda
Copy link
Member

I'll try to tackle these issues today...

@pslacerda
Copy link
Member

I'm very sorry, I was wrong about the conflict. But now we have a different HEAD in master. Can you redo you work on top of master again?

@speleo3
Copy link
Contributor

speleo3 commented Jun 21, 2025

As far as I can tell, #152 fixed the multi-object problem and this pull request can be closed. Can you confirm?

Here is an example:

fab ACDEFG, m1
fab HIKLMN, m2
findseq DEF, *, foundSeq
assert cmd.count_atoms("foundSeq and m1") == 47
assert cmd.count_atoms("foundSeq and m2") == 0

Result:

  • Before #152 merged: Incorrect selection in m2, second assertion fails
  • After #152 merged: Correct selection, both assertions pass

@e-osipov
Copy link
Author

e-osipov commented Jun 26, 2025

I see that the problem was fixed and it works correctly when the same selection is found in multiple objects multiple times.

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