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

Metadata GDSC annotation improvements #625

Merged
merged 5 commits into from
Jun 10, 2024
Merged

Metadata GDSC annotation improvements #625

merged 5 commits into from
Jun 10, 2024

Conversation

Lilly-May
Copy link
Collaborator

Description of changes

  • Fixed the logger error message in the cell line metadata annotation. The code will still fail, but now the informative logging message will be shown.
  • The parameter specifying the GDSC dataset can now also be provided as a string. Feel free to reject this change, as I might be the only user making the mistake of providing it as a string instead of an integer. The latter causes the method to always use GDSC dataset 2 without specifically stating it, which might be overlooked by the user. I also added a test that the parameter is set to 1 or 2 and otherwise throw an error.
  • The most important change is in the annotate_from_gdsc method, which now uses the correct GDSC data to compute. Previously, the code would fail for me if GDSC2 was used.
  • Adapted the Statsmodel example. I believe an old class name might have been used here. It's still not a working example as the adata is not defined, but I think it is much easier for users to understand now.

@Lilly-May Lilly-May marked this pull request as ready for review June 8, 2024 12:45
@Lilly-May Lilly-May requested review from wxicu and Zethson June 8, 2024 12:45
Copy link
Member

@Zethson Zethson 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! The GDSC string change is fine with me. I would also even argue that it should be a Literal or the two string options @wxicu ?

Copy link
Collaborator

@wxicu wxicu left a comment

Choose a reason for hiding this comment

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

Thanks for the improvement! Literal of gdsc dataset sounds good to me

@Zethson
Copy link
Member

Zethson commented Jun 10, 2024

@Lilly-May can you change this so that we use string literals for the dataset names? Maybe Literal["gdsc_1", "gdsc_2"] or something of that sort? Then we can merge it

@Lilly-May
Copy link
Collaborator Author

Sure! But you still want it to work with integers as well, right? So something like Literal["gdsc_1", "gdsc_2"] | Literal[1, 2]?

@Zethson
Copy link
Member

Zethson commented Jun 10, 2024

I'd drop support for integers

@Lilly-May Lilly-May merged commit c69acea into main Jun 10, 2024
3 of 7 checks passed
@Lilly-May Lilly-May deleted the de_improvements branch August 30, 2024 17:04
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