I tried with running preprocessing with this branch. It seems it still tries to check for all chains in PDB?
Unfortunately some PDBs have errors in chain names, so there are inconsistencies between no. of PDB chains and no. of PSSMs.
I'll do the checks again

The changes work. This pull request can be closed.
However, there should be a provision to include other pssm chains in case the radius includes multiple chains. can be dealt in #429

The changes work. This pull request can be closed. However, there should be a provision to include other pssm chains in case the radius includes multiple chains. can be dealt in #429

It works because it now fills the pssm feature with 0s. So this PR actually deals with both #428 and #429, as they are closely related. In retrospect, maybe that wasn't ideal, because now things are getting a bit confused.
I will await decision on #429 to know whether we actually want to fill it with 0s, or want to force pssm files to be present for all chains and then either create separate PRs or resolve both issues here.

I am making a small dataset with variants that have neighbours in multiple chains.
I suppose that may be the only way to test consequence of 0s or non-0s in predictions (non-0s will be ideal for these cases)

I am making a small dataset with variants that have neighbours in multiple chains. I suppose that may be the only way to test consequence of 0s or non-0s in predictions (non-0s will be ideal for these cases)

I think this will only tell you something about your particular use case, but we want a solution that is generally applicable to anyone using the package.
So in a wider context, we need to make a decision whether we want users to provide pssm files for all chains included in the network (i think preferable) or whether we allow them to omit pssm files for some chains and then the feature gets filled with 0s for those chains.
Note that for the second case, it would put 0s for whatever chain/chains is/are omitted, without differentiating what the main chain and the secondary chain is, etc.
For the first option (required pssm files), users can always just generate pssm files with all 0s if needed.

My test was for the general applicability. By omitting pssm files and adding 0s we would treat such variants as though they are solvent-exposed variants- which is not quite right, although I am not sure how many such cases are there. I have never tested such a thing with deeprank-mut, but it is expected and I do note that solvent-exposed variants are treated differently in network.

I would prefer to include all chains within the radius, so may be user can definitely include all relevant chains. Rest can be 0

so do I understand correctly that all residues from chains within the radius should be included in the network and that for those chains a pssm file should be required (with an error message telling you which files are missing if they are not provided)?

@rgayatri , can you please test on the current branch if it still works as intended for you?

Note that Query objects now have an optional argument suppress_pssm_errors

I think we're missing a test that uses suppress_pssm_errors=True. Indeed I was wondering, what happens if the errors are suppressed and pssm files are missing or incorrect? What will conservation-related features contain?

lines 390 and 413 of test_query.py ;-)

Good question what the hdf5 file will then contain. I will look into it.

I think we're missing a test that uses suppress_pssm_errors=True. Indeed I was wondering, what happens if the errors are suppressed and pssm files are missing or incorrect? What will conservation-related features contain?

lines 390 and 413 of test_query.py ;-)

Good question what the hdf5 file will then contain. I will look into it.

Oh right! What if we explicitly set suppress_pssm_errors in the query initialization (maybe redefining the query in the tests)? This way is clearer how such a parameter can be used, instead of setting the internal _suppress. Since we don't have explicit examples about suppress_pssm_errors, can be good to have it in the tests

I think we're missing a test that uses suppress_pssm_errors=True. Indeed I was wondering, what happens if the errors are suppressed and pssm files are missing or incorrect? What will conservation-related features contain?

lines 390 and 413 of test_query.py ;-)
Good question what the hdf5 file will then contain. I will look into it.

Oh right! What if we explicitly set suppress_pssm_errors in the query initialization (maybe redefining the query in the tests)? This way is clearer how such a parameter can be used, instead of setting the internal _suppress. Since we don't have explicit examples about suppress_pssm_errors, can be good to have it in the tests

I did it this way to make sure that the test uses the same instance of the object that failed if the error suppression was not turned on (a few lines above). This way, if someone modifies the Query object in the test, we are sure that both tests run the same instance.
Do you think I should add an additional test where it is set upon initialization?

I think we're missing a test that uses suppress_pssm_errors=True. Indeed I was wondering, what happens if the errors are suppressed and pssm files are missing or incorrect? What will conservation-related features contain?

lines 390 and 413 of test_query.py ;-)

Good question what the hdf5 file will then contain. I will look into it.

OK, I looked into it:

• for incorrect entries in the pssm: it just used the "wrong" data. So the pssm data that is associated to the position number of that residue, even though it is the incorrect residue (@rgayatri is this the behavior that you want?)
• for missing entries in the pssm:
  • if the entry is not part of the graph, it doesn't complain
  • if the entry is a part of the graph, then it fails at a later step (so check_pssm passes, but then an error is thrown I think when it tries to write data into that key).

The latter point made me realize that it still checks all residues from the chain, irrespective of whether they are a part of the graph. I will rewrite so that it only checks those residues that are part of the graph.

@rgayatri , what behavior do you expect if a residue is missing from a pssm file? All 0s?

At the end of the day, all these tests make me feel that we just don't want people to use bad pssm files. I don't like the concept of being able to suppress the error, because now we need to hack ad hoc solutions to deal with bad data.

I think we're missing a test that uses suppress_pssm_errors=True. Indeed I was wondering, what happens if the errors are suppressed and pssm files are missing or incorrect? What will conservation-related features contain?

lines 390 and 413 of test_query.py ;-)
Good question what the hdf5 file will then contain. I will look into it.

OK, I looked into it:

• for incorrect entries in the pssm: it just used the "wrong" data. So the pssm data that is associated to the position number of that residue, even though it is the incorrect residue (@rgayatri is this the behavior that you want?)

• for missing entries in the pssm:

  • if the entry is not part of the graph, it doesn't complain
  • if the entry is a part of the graph, then it fails at a later step (so check_pssm passes, but then an error is thrown I think when it tries to write data into that key).

The latter point made me realize that it still checks all residues from the chain, irrespective of whether they are a part of the graph. I will rewrite so that it only checks those residues that are part of the graph.

@rgayatri , what behavior do you expect if a residue is missing from a pssm file? All 0s?

At the end of the day, all these tests make me feel that we just don't want people to use bad pssm files. I don't like the concept of being able to suppress the error, because now we need to hack ad hoc solutions to deal with bad data.

I agree, ideally, a user shouldn't be able to suppress the error indeed and should take care of providing the correct files. But if @rgayatri thinks that it makes sense to fill bad data with 0s, then we can do that.

Good that you noticed that all residues were checked instead of the relevant ones only @DaniBodor, and indeed better to make it check only those residues that are part of the graph.

This PR is stale because it has been open for 14 days with no activity.

I believe that at this point the current PR is functional. If fed with accurate data, everything works as expected. Otherwise errors can be suppressed if absolutely necessary, but it can still fail if this touches data points that are part of the graph. This is okay behavior, as we do not want users to train with unexpected/unexplainable results.
As such, I will be merging and closing this PR. If there are any remaining problems to be solved about this, we can open new issues about them.