Provide charge model #245
Provide charge model #245
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR provides a unified interface and updated weights for the charge model while ensuring consistent model handling across different charge state configurations. Key changes include initializing the charge model with global supported charge constants, adding training and prediction methods specific to the charge model, and updating default settings and test notebooks to align with these changes.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| peptdeep/pretrained_models.py | Instantiates and loads a unified charge model and adds new training and prediction methods. |
| peptdeep/model/charge.py | Updates the charge model interface to use global charge constants and adjusts indexing logic. |
| peptdeep/constants/default_settings.yaml | Updates model zip names/URLs and adds hyperparameters for charge model training. |
| nbs_trials/test_charge_model.ipynb | Provides a notebook for testing charge model training and prediction. |
peptdeep/model/charge.py
Outdated
| min_precursor_charge - self.min_predict_charge : max_precursor_charge | ||
| - self.min_predict_charge | ||
| min_precursor_charge | ||
| - self.min_supported_predict_charge : max_precursor_charge |
There was a problem hiding this comment.
I'm not sure if I understand what's happening here :D
Is it user requested or model supported clipping?
Why don't we substract a constant as I would expect that the indicators are always 10elements?
There was a problem hiding this comment.
I guess the constants are loaded into the variables. Should we maybe use the constants in the first place?
| super().__init__( | ||
| num_target_values=max_charge - min_charge + 1, | ||
| num_target_values=MAX_SUPPORTED_CHARGE - MIN_SUPPORTED_CHARGE + 1, |
There was a problem hiding this comment.
can you add a constant for this? Like NUM_SUPPORTED_CHARGE
| df: pd.DataFrame, | ||
| charge_prob_cutoff: float = 0.3, | ||
| min_precursor_charge: int = 1, | ||
| max_precursor_charge: int = 6, |
There was a problem hiding this comment.
The docstring says the default is 10 :D
| DataFrame containing the "charge_indicators" and "charge_probs" columns. | ||
| charge_prob_cutoff : float, optional | ||
| Cutoff for charge prediction, by default 0.3 | ||
| min_precursor_charge : int, optional |
There was a problem hiding this comment.
can you add that these are the user requested charges and that the model is limited to charge 1-10
peptdeep/pretrained_models.py
Outdated
| Maximum precursor charge. | ||
|
|
||
| charge_prob_cutoff : float | ||
| Charge probability cutoff. |
There was a problem hiding this comment.
Can you add something like: "Set to None to predict all charges"
Uploaded the char model weights and provided a unified interface for the charge model where the architecture (prediction head) doesn't change based on the number of charge states available in the training data.