[ENH] PseAAC encoding algorithm

[satvshr]

Solves #28.
Merge before #13.
Implements the encoding algorithm that will take the string of the target protein as input and output a feature vector containing its physicochemical properties based on the distance between each amino acid in that protein.

[satvshr]

To do: Add tests comparing it to the implementation in the official AptaNet repository.

[fkiraly]

Looks like a quite simple algorithm?

Can you kindly change this to a class, and move the many internal functions to private methods? Functions inside functions are not good style.

Further, there are also a number of loops, which look like they can be vectorized using numpy.

Finally, it looks like a lot of stuff gets precomputed every time, i.e., the prop_groups. I would separate this out from the algorithm, and think about caching the results.

[fkiraly]

regarding code structure: this should move in a folder for packaging, I think currently we are using pyaptamer and not src.

[satvshr]

Looks like a quite simple algorithm?

Yup! Logically, it is not as complex as I initially thought it was.

Can you kindly change this to a class, and move the many internal functions to private methods? Functions inside functions are not good style.

Will get to it.

it looks like a lot of stuff gets precomputed every time, i.e., the prop_groups. I would separate this out from the algorithm, and think about caching the results.

prop_groups is not getting computed but I think you are talking about the normalize_properties function which uses fixed values and hence does not need to be computed? I thought that the actual physicochemical values for all the properties should also be mentioned hence I introduced the function, Should I add 21 more variables for the normalized physicochemical values?

[fkiraly]

prop_groups is not getting computed

I meant everything depending on prop_groups and only on that - since prop_groups are hard-coded, the normalized prop_groups will always yield the same result.

I thought that the actual physicochemical values for all the properties should also be mentioned hence I introduced the function, Should I add 21 more variables for the normalized physicochemical values?

Yes, I think we should have the physiochemical values stored somewhere - the pattern I would personally use is compute it once and hard-code the result. But then also add a test that the hard-coded results are equal to the computation from the original values, so the compute load for the "verification" is in the tests, and not every time the algorithm gets called (where it is unnecessary).

Plus, of course, ample comments, maybe a getter function whose docstrings explain what the values exactly are that it returns, with a pointer to the tests and the original physiochemical tables.

[fkiraly]

Great!

May I request some structural changes:

• move the input from __init__ to vectorize - I assume that is possible?
• move the checking whether a string is an amino-acid string to a utils module. Use the set of letters instead of the dict.
• move the self.P1 etc to a separate private module where thes stay hard-coded as constants.
• _average_aa is not pythonic, this can be simplified
• various methods look like they can be vectorized or replaced by numpy drop-ins.
• where did you move the precomputations?

[satvshr]

move the input from __init__ to vectorize - I assume that is possible?

Done.

move the checking whether a string is an amino-acid string to a utils module. Use the set of letters instead of the dict.

Done.

move the self.P1 etc to a separate private module where they stay hard-coded as constants.

Done.

_average_aa is not pythonic, this can be simplified

Done.

various methods look like they can be vectorized or replaced by numpy drop-ins.

Done.

where did you move the precomputations?

All the values with variable names P{i} are the raw values, variable named NP{i} are the normalized ones. I also moved the normalized values to the private file.

[fkiraly]

above you give numbers to the properties, that is good since it is specific. For consistency, I would suggest you use a numbered list instead of a bullet point list then.

Minor formatting suggestin at this point: ensure before lists (bullet points, numbered etc) and after there are two newlinds, i.e., an empty line between the text and first item etc. The reason for this is, rst expects this.

[satvshr]

Done.

[fkiraly]

Remove from bullet point and move to top level. This is not another set of features but explanation of how they sum up to everything.

[satvshr]

Done.

[fkiraly]

only small docstring comments, above

[satvshr]

After 110 comments (most of them about docstrings annoyingly) and this PR almost being merged. I propose some changes if we want to make PSeAAC a standalone algorithm:

1. Move all "21 physicochemical properties" references out of PSeAAC as in the paper, they mention having a total of 24 properties, out of which different combinations of them give the best results for different k values.
2. Update the helper function aa_props and the PSeAAC class to accept indices of physicochemical properties user wants to use.

[satvshr]

Was browsing through google to find the original PSeAAC paper by Chou (not available publically) and found this, it is not installable via pip but the download link can be found here, along with a user guide.

[fkiraly]

Question, why did you extend the specification of the class? With group_props etc.
Is this "feature creep"? I feel this should have been a separate pull request, if at all.

The docstring is also very hard to understand what it means.

[satvshr]

Question, why did you extend the specification of the class? With group_props etc. Is this "feature creep"? I feel this should have been a separate pull request, if at all.

The algorithm I had implemented was wrong because we were assuming there were only 21 property groups to use, and those groups would be grouped into groups of 3. This is valid only for the best experiment from AptaNet, not for PSeAAC in general.

The docstring is also very hard to understand what it means.

Forgot to edit the docstrings after the final draft, will do so.

[fkiraly]

The algorithm I had implemented was wrong because we were assuming there were only 21 property groups to use, and those groups would be grouped into groups of 3. This is valid only for the best experiment from AptaNet, not for PSeAAC in general.

Can you please explain that?

I would suggest we split this PR: turn this back into the mergeable almost-approved state where we had the 21 property groups; move the changes after that into a new PR. I think we need to discuss things like the API design and the abstractions here, I do not think the current ones are good choices.

The danger is we have an ever-growing PR before we have merged anything. It is best practice to not re-scope or widen scope of issues or PR in the middle of it.

[fkiraly]

Thanks.

Since the tests do not run on CI, can you confirm they run locally?

[satvshr]

Thanks.

Since the tests do not run on CI, can you confirm they run locally?

Tests pass locally.

[satvshr]

FINALLY! 🎉