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.

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

simplify-dataset-init #333

Merged
merged 7 commits into from
Sep 20, 2024
Merged

simplify-dataset-init #333

merged 7 commits into from
Sep 20, 2024

Conversation

mschwoer
Copy link
Contributor

@mschwoer mschwoer commented Sep 18, 2024

Move some of the init logic to a new class.

Now the DataSet class should contain next to no logic anymore.

@mschwoer mschwoer marked this pull request as ready for review September 18, 2024 16:38
self.software: str = loader.software
self._gene_names: str = loader.gene_names

self._intensity_column: Union[str, list] = (
Copy link
Contributor Author

@mschwoer mschwoer Sep 18, 2024

Choose a reason for hiding this comment

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

note that this was different before:
loader.intensity_column was used to create the mat object, and only afterwards overwritten in the case of a generic loader .. I suppose this was not intended

Copy link
Collaborator

@boopthesnoot boopthesnoot left a comment

Choose a reason for hiding this comment

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

Thank you! Looks good

sample_column: Optional[str] = None,
):
"""Create DataSet

Args:
loader (_type_): loader of class AlphaPeptLoader, MaxQuantLoader, DIANNLoader, FragPipeLoader, SpectronautLoader
metadata_path (str, optional): path to metadata file. Defaults to None.
metadata_path (str or pd.DataFrame, optional): path to metadata file or an actual df. Defaults to None.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
metadata_path (str or pd.DataFrame, optional): path to metadata file or an actual df. Defaults to None.
metadata_path_or_df (str or pd.DataFrame, optional): path to metadata file or an actual df. Defaults to None.

sorry to be this person hahaha

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is indeed a good suggestion to make this api better .. in the long run, I'd implement two distinct parameters (path and df) .. did the whole change here: #336

Base automatically changed from decouple-dataset-from-plots to development September 20, 2024 21:48
@mschwoer mschwoer merged commit 629b03c into development Sep 20, 2024
5 checks passed
@mschwoer mschwoer deleted the simplify-dataset-init branch September 20, 2024 21:49
@JuliaS92
Copy link
Collaborator

This looks good to me, wlthough I am not sure the functionalitz provided by the dataset factory really need to/should be separated, as this defines the core of the DataSet.

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.

3 participants