Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

changed time_cutoff option #89

Merged
merged 7 commits into from
Mar 26, 2024
Merged

changed time_cutoff option #89

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f723356
changed time_cutoff option
kierandidi Mar 23, 2024
707b898
added to CHANGELOG
kierandidi Mar 23, 2024
cb32b81
changed PDBDataset arguments
kierandidi Mar 24, 2024
48c64e8
Changed yaml file arg order
kierandidi Mar 24, 2024
6b1912c
reorder arguments
kierandidi Mar 25, 2024
878fa96
add check for time interval
kierandidi Mar 26, 2024
7a3875d
changed time intervall check
kierandidi Mar 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* Improves support for datamodules with multiple test sets. Generalises this to support GO and FOLD. Also adds multiple seq ID.-based splits for GO. [#72](https://github.com/a-r-j/ProteinWorkshop/pull/72)
* Add redownload checks for already downloaded datasets and harmonise pdb download interface [#86](https://github.com/a-r-j/ProteinWorkshop/pull/86)
* Remove remaining errors from PDB dataset change
* Add option to create pdb datasets with sequence-based splits [#88](https://github.com/a-r-j/ProteinWorkshop/pull/88)
* Add option to create pdb datasets with sequence-based splits [#88](https://github.com/a-r-j/ProteinWorkshop/pull/88) as well as time-based splits [#89](https://github.com/a-r-j/ProteinWorkshop/pull/89)

### Models

Expand Down
5 changes: 3 additions & 2 deletions proteinworkshop/config/dataset/pdb.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ datamodule:
remove_non_standard_residues: True # Include only proteins containing standard amino acid residues
remove_pdb_unavailable: True # Include only proteins that are available to download
train_val_test: [0.8, 0.1, 0.1] # Cross-validation ratios to use for train, val, and test splits
split_type: "sequence_similarity" # Split sequences by sequence similarity clustering, other option is "random"
split_sequence_similiarity: 0.3 # Clustering at 30% sequence similarity (argument is ignored if split_type="random")
split_type: "sequence_similarity" # Split sequences by sequence similarity clustering, other options are "random" and "time_cutoff"
split_sequence_similiarity: 0.3 # Clustering at 30% sequence similarity (argument is ignored if split_type!="sequence_similarity")
split_time_frames: ["2020-01-01", "2021-01-01", "2023-03-01"] # Time-cutoffs for train, val and test set (argument is ignored if split_type!="time_cutoff")
Copy link
Owner

Choose a reason for hiding this comment

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

Better to set to null? What do you thinl?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you mean split_time_frames? Thought it would be good to have it in there so that users can see what format is required

Copy link
Collaborator

@amorehead amorehead Mar 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Perhaps you can null out split_time_frames but leave your example in the in-line comment as follows:

split_time_frames: null # Time-cutoffs for train, val and test set (argument is ignored if split_type!="time_cutoff") - e.g., ["2020-01-01", "2021-01-01", "2023-03-01"]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Copy link
Collaborator

Choose a reason for hiding this comment

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

LGTM

overwrite_sequence_clusters: False # Previous clusterings at same sequence similarity are reused and not overwritten
19 changes: 14 additions & 5 deletions proteinworkshop/datasets/pdb_dataset.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import hydra
import omegaconf
import numpy as np
import os
import pandas as pd
import pathlib
Expand Down Expand Up @@ -30,9 +31,10 @@ def __init__(
remove_non_standard_residues: bool,
remove_pdb_unavailable: bool,
train_val_test: List[float],
split_type: Literal["sequence_similarity", "random"],
split_type: Literal["sequence_similarity", "time_cutoff", "random"],
split_sequence_similiarity: int,
overwrite_sequence_clusters: bool
overwrite_sequence_clusters: bool,
split_time_frames: List[str]
Copy link
Owner

Choose a reason for hiding this comment

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

Should be Optional[List[str]]. This arg probs needs a check for datetime format.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In theory it is optional, but we cannot specify a default option as it is later in the argument list. If we want to make the type hint optional, should we move it up the list and give it a default value?

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, I'm generally in favour of fewer args (and not passing in things that aren't used since it can be confusing). What do you think about a pattern where we can pass the time splits into train_test_split instead of the list of floats and the behaviour is controlled by split_type (with appropriate error catching)?

Copy link
Owner

Choose a reason for hiding this comment

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

cc @amorehead

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With pass the time splits into train_test_split, do you mean giving tuples of (split_ratio, split_time_cutoff) if the time version is chosen for example? And if the time version is not chosen, one would just take the first element of that tuple, with the default for the second being None?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I think clarification would be helpful here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Was not sure what exactly you meant @a-r-j, but for now, I just reordered arguments so that we can give defaults to these and the user does not need to specify them. Does that work for you?

):
self.fraction = fraction
self.molecule_type = molecule_type
Expand All @@ -52,6 +54,7 @@ def __init__(
self.split_type = split_type
self.split_sequence_similarity = split_sequence_similiarity
self.overwrite_sequence_clusters = overwrite_sequence_clusters
self.split_time_frames = [np.datetime64(date) for date in split_time_frames]
self.splits = ["train", "val", "test"]

def create_dataset(self):
Expand Down Expand Up @@ -128,9 +131,15 @@ def create_dataset(self):
elif self.split_type == "sequence_similarity":
log.info(f"Splitting dataset via sequence-similarity split into {self.train_val_test}...")
log.info(f"Using {self.split_sequence_similarity} sequence similarity for split")
pdb_manager.cluster(min_seq_id=self.split_sequence_similarity, update=True)
splits = pdb_manager.split_clusters(
pdb_manager.df, update=True, overwrite = self.overwrite_sequence_clusters)
pdb_manager.cluster(min_seq_id=self.split_sequence_similarity, update=True,
overwrite = self.overwrite_sequence_clusters)
splits = pdb_manager.split_clusters(pdb_manager.df, update=True)

elif self.split_type == "time_cutoff":
log.info(f"Splitting dataset via time_cutoff split into {self.train_val_test}...")
log.info(f"Using {self.split_time_frames} dates for split")
pdb_manager.split_time_frames = self.split_time_frames
splits = pdb_manager.split_by_deposition_date(df=pdb_manager.df, update=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Very nice!


log.info(splits["train"])
return splits
Expand Down
Loading