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

Tweak print method to show altExpNames #85

Closed
wants to merge 5 commits into from

Conversation

Biomiha
Copy link

@Biomiha Biomiha commented Aug 23, 2023

Add printing of altExpNames if they exist or NULL if there is nothing.

@stemangiola stemangiola linked an issue Aug 23, 2023 that may be closed by this pull request
@stemangiola stemangiola self-requested a review August 23, 2023 22:18
Copy link
Owner

@stemangiola stemangiola left a comment

Choose a reason for hiding this comment

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

Thanks! Well done.

For some reason, you have conflicts to solve (see above). Maybe recent PR changes have the same lines you change. Please have a look and solve the conflicts.

Also, I see you have two branches, but you are doing the PR just for master. Not sure if this is intended.

Copy link
Owner

@stemangiola stemangiola left a comment

Choose a reason for hiding this comment

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

Also, please add a unit test, add a dummy "alternative experiment", and verify that the print looks as expected.

paste(assay_names, collapse=", ")
), after=1)
append(sprintf(
"\033[90m Features=%s | Cells=%s | Assays=%s | altExpNames=%s\033[39m",
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks.

I bit more elegant would be

  • Replace altExpNames with Assays_alternative
  • Conditionally add "| altExpNames=%s" if alternative experiment exists

Copy link
Author

Choose a reason for hiding this comment

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

Fair point. I had considered adding the information on a conditional basis but personally I prefer to know even if it's empty. The standard SCE print method outputs altExpNames(0): when empty and altExpNames(1): Antibody Capture when it is populated (Antibody Capture being the name of the alternative experiment and can be any string). If you feel you would rather keep it only when it is non-NULL I can do that quite easily.
In relation to the naming, it actually refers to the name of the experiment rather than the names of the assays, i.e. if it exists you will still have counts and normcounts or other as the assay names within the altExp slot. How about saying Other_experiments=?
I will also add the unit tests and other missing bits whilst I am at it.

Thanks.

Copy link
Owner

@stemangiola stemangiola Aug 24, 2023

Choose a reason for hiding this comment

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

I see.

let's try to generalise the concept of experiment and assay, still hinting at the non-modular nature of singlecellexperiment.

So I am in favour of printing the experiments within the object, if they are 1,2,3, independently whether they are base or alternative.

having said that, there are a lot of ways to go about that. One possibility is to do

Assays= base:counts,logcounts; Antibody capture:counts,logcounts; ADT:counts

Other ideas are welcome!

Copy link
Owner

@stemangiola stemangiola Aug 25, 2023

Choose a reason for hiding this comment

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

@Biomiha, I realised that the sentence "OK, the goal here is to provide an interface that promotes modularity. " sounded very bad in writing; the tone I was thinking was completely different from how you would read it!

I used a poor form to give the broad indication that "the tidy transcriptomics thrives to giving modularity (which sometimes diverges from the Bioc framework)."

I hope you did not take it personally!

Copy link
Author

Choose a reason for hiding this comment

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

No dude, absolutely not. I work in biotech 😄 .
I agree with what you've said. Am working on implementation, just been very busy lately.

@Biomiha Biomiha closed this Aug 27, 2023
@Biomiha Biomiha deleted the new-altExpNames branch August 27, 2023 16:07
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.

tidySCE Functionality to see and access the altExp slot
2 participants