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

Fix messaging for data processing changes #983

Merged
merged 1 commit into from
Feb 16, 2024
Merged

Fix messaging for data processing changes #983

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 9 additions & 4 deletions src/pages/experiments/[experimentId]/data-processing/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -695,10 +695,15 @@ const DataProcessingPage = ({ experimentId, experimentData }) => {
Your navigation within Cellenics will be restricted during this time.
Do you want to start?
</p>
<Alert
message='Note that you will lose all of your annotated cell sets.'
type='warning'
/>
{
!(changedQCFilters.size === 1 && changedQCFilters.has('embeddingSettings'))
&& (
<Alert
message='Note that you will lose your previous Louvain or Leiden clusters.'
Copy link
Contributor

@alexvpickering alexvpickering Feb 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@ogibson -- you will also lose custom cell sets, automatically annotated cell sets, etc

please have a bioinformatician review changes like this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did double check this specifically for custom cell sets and verified that we weren't losing them. Haven't checked for automatically annotated cell sets yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue came up when trying to merge this to our fork. Martin pointed out that we weren't actually losing all cell sets and he specifically asked for this change (also to fix the fact that running embedding doesn't remove annotations).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Automatically annotated cellsets aren't lost either. Not when rerunning clustering nor when changing QC settings.

Copy link
Contributor

@alexvpickering alexvpickering Feb 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes but this warning is for any changes to QC -- I agree we could differentiate but it would get complicated:

  1. only embedding (UMAP vs TSNE) settings --> lose nothing
  2. only changing clustering settings --> lose louvain/leiden clustering only
  3. changing anything else in QC --> lose all annotations

I think until we implement this complicated logic for whether to show a warning, the cautionary principle is best (more likely to be upset if lose annotations and didn't know that would -- probably won't care if told that would lose annotations and didn't)

Copy link
Contributor Author

@ogibson ogibson Feb 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I tested changing other settings in QC, and you still don't lose annotations (only louvain clusters).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also thought we would lose them, but I believe this was changed at some point.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh nice! Could have swarn I checked that but happy to be wrong -- apologies for the misunderstanding :)

type='warning'
/>
)
}
</Modal>
)
)}
Expand Down
Loading