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

Sera/issue random sample size #3

Merged
merged 5 commits into from
Jun 2, 2024
Merged

Conversation

serareif
Copy link
Collaborator

@serareif serareif commented Jun 2, 2024

No description provided.

@serareif serareif requested a review from nictru June 2, 2024 15:09
@serareif serareif self-assigned this Jun 2, 2024
Copy link
Collaborator

@nictru nictru left a comment

Choose a reason for hiding this comment

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

The problem here is not that the slider is not being updated. The update does happen as expected here. Adding an additional location where the slider is updated introduces unnecessary complexity.

The problem is that the random sample function will be executed whenever either the slider or the adata is updated.
So the following happens here if the adata is updated:

  1. random_sample and slider_sample at the same time
  2. random_sample again because the slider was updated

The problem will only occur when the random_sample is called the first time, as the slider has not been updated yet but the adata has

A solution could look as simple as this:

@reactive.effect
def random_sample():
    adata = _adata_qc.get()
    sample_size = input['random_sample_size'].get()
    
    if adata is None:
        return

    adata_sample = adata[np.random.choice(adata.obs.index, min(sample_size, len(adata.obs)), replace=False)]
    _adata_sample.set(adata_sample)

@nictru
Copy link
Collaborator

nictru commented Jun 2, 2024

Also please add a patch release to 0.2.1 here

@@ -76,12 +76,15 @@ def return_of_adata_meta():
@reactive.effect
def random_sample():
adata = _adata_qc.get()
if adata is None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be sufficient to check only once if adata is None

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

okee

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

@nictru nictru merged commit 9ac5941 into main Jun 2, 2024
1 check passed
@nictru nictru deleted the sera/issue_random_sample_size branch June 2, 2024 19:58
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.

2 participants