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

added pre-qc-failed status filter #24

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

KateSakharova
Copy link
Contributor

@KateSakharova KateSakharova commented Dec 17, 2024

Added filtering for not re-trying pre-qc failed assemlies and amplicons

@KateSakharova KateSakharova force-pushed the fix/add-pre-qc-failed-filter branch from a9a01d7 to 4dea336 Compare December 17, 2024 14:14
@KateSakharova KateSakharova marked this pull request as ready for review December 17, 2024 14:45
@@ -272,6 +272,7 @@ def default_status(cls):
cls.ASSEMBLY_UPLOADED: False,
cls.ASSEMBLY_UPLOAD_FAILED: False,
cls.ASSEMBLY_UPLOAD_BLOCKED: False,
cls.PRE_ASSEMBLY_QC_FAILED: False,
Copy link
Member

@mberacochea mberacochea Dec 19, 2024

Choose a reason for hiding this comment

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

Please put "cls.PRE_ASSEMBLY_QC_FAILED: False," after "cls.ASSEMBLY_STARTED: False," :)

Copy link
Member

@mberacochea mberacochea left a comment

Choose a reason for hiding this comment

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

Good catch.. not sure if we should rename the analyses QC to PRE-QC

@@ -504,7 +505,7 @@ class AnalysisStates(str, Enum):
ANALYSIS_COMPLETED = "analysis_completed"
ANALYSIS_BLOCKED = "analysis_blocked"
ANALYSIS_FAILED = "analysis_failed"
ANALYSIS_QC_FAILED = "analysis_qc_failed"
ANALYSIS_PRE_QC_FAILED = "analysis_pre_qc_failed"
Copy link
Member

Choose a reason for hiding this comment

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

For analyses we don't have a "pre-qc".. it's just QC, right?

@@ -126,6 +126,7 @@ def get_assemblies_to_attempt(study: analyses.models.Study) -> List[Union[str, i
**{
f"status__{analyses.models.Assembly.AssemblyStates.ASSEMBLY_COMPLETED}": False,
f"status__{analyses.models.Assembly.AssemblyStates.ASSEMBLY_BLOCKED}": False,
f"status__{analyses.models.Assembly.AssemblyStates.PRE_ASSEMBLY_QC_FAILED}": False,
Copy link
Member

Choose a reason for hiding this comment

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

This one should go first; it doesn't change how it works—just improves readability

Copy link
Member

Choose a reason for hiding this comment

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

A little manager mixin helper would be nice for building these more readably:

class SelectByStatusManagerMixin:
    STATUS_FIELDNAME = "status"
    
    def filter_by_statuses(self, statuses_to_be_true: List[Union[str, Enum]] = None, statuses_to_be_false: List[Union[str, Enum]] = None):
        """
        Filter queryset by a combination of statuses in the objects status json field.
        """
        filters = {}
        if statuses_true:
            filters = {
                f"{self.STATUS_FIELDNAME}__{must_be}": True
                for must_be in statuses_true
            }
        if statuses_false:
            filters.update({
                f"{self.STATUS_FIELDNAME}__{must_not_be}": True
                for must_not_be in statuses_false
            })
        return super().get_queryset().filter(**filters)

Then we can do class AssemblyManager(SelectByStatusManagerMixin, ENADerivedManager): ...

and select more readably, like

assemblies_worth_trying = study.assemblies_reads.filter_by_statuses(
    statuses_to_be_false=[
        analyses.models.Assembly.AssemblyStates.ASSEMBLY_COMPLETED,
        analyses.models.Assembly.AssemblyStates.ASSEMBLY_BLOCKED,
        analyses.models.Assembly.AssemblyStates.PRE_ASSEMBLY_QC_FAILED,
    ]
).values_list("id", flat=True)

(or maybe better yet include an exclude_by_statuses chainable method on the mixin).

Happy to add this later as it will need a couple of new tests. Just documenting whilst it came to mind!

@@ -126,6 +126,7 @@ def get_assemblies_to_attempt(study: analyses.models.Study) -> List[Union[str, i
**{
f"status__{analyses.models.Assembly.AssemblyStates.ASSEMBLY_COMPLETED}": False,
f"status__{analyses.models.Assembly.AssemblyStates.ASSEMBLY_BLOCKED}": False,
f"status__{analyses.models.Assembly.AssemblyStates.PRE_ASSEMBLY_QC_FAILED}": False,
Copy link
Member

Choose a reason for hiding this comment

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

A little manager mixin helper would be nice for building these more readably:

class SelectByStatusManagerMixin:
    STATUS_FIELDNAME = "status"
    
    def filter_by_statuses(self, statuses_to_be_true: List[Union[str, Enum]] = None, statuses_to_be_false: List[Union[str, Enum]] = None):
        """
        Filter queryset by a combination of statuses in the objects status json field.
        """
        filters = {}
        if statuses_true:
            filters = {
                f"{self.STATUS_FIELDNAME}__{must_be}": True
                for must_be in statuses_true
            }
        if statuses_false:
            filters.update({
                f"{self.STATUS_FIELDNAME}__{must_not_be}": True
                for must_not_be in statuses_false
            })
        return super().get_queryset().filter(**filters)

Then we can do class AssemblyManager(SelectByStatusManagerMixin, ENADerivedManager): ...

and select more readably, like

assemblies_worth_trying = study.assemblies_reads.filter_by_statuses(
    statuses_to_be_false=[
        analyses.models.Assembly.AssemblyStates.ASSEMBLY_COMPLETED,
        analyses.models.Assembly.AssemblyStates.ASSEMBLY_BLOCKED,
        analyses.models.Assembly.AssemblyStates.PRE_ASSEMBLY_QC_FAILED,
    ]
).values_list("id", flat=True)

(or maybe better yet include an exclude_by_statuses chainable method on the mixin).

Happy to add this later as it will need a couple of new tests. Just documenting whilst it came to mind!

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