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

Feature/SCKAN-364 - Add statement preview row to the export + persist statement preview suffix and prefix #404

Merged
merged 10 commits into from
Jan 17, 2025
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
50 changes: 2 additions & 48 deletions backend/composer/api/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@
)
from ..services.connections_service import get_complete_from_entities_for_destination, \
get_complete_from_entities_for_via
from ..services.statement_service import create_statement_preview
from ..services.errors_service import get_connectivity_errors
from ..utils import join_entities


# MixIns
Expand Down Expand Up @@ -644,53 +644,7 @@ def get_entities_journey(self, instance):
def get_statement_preview(self, instance):
if 'journey' not in self.context:
self.context['journey'] = instance.get_journey()
return self.create_statement_preview(instance, self.context['journey'])

def create_statement_preview(self, instance, journey):
sex = instance.sex.sex_str if instance.sex else None

species_list = [specie.name for specie in instance.species.all()]
species = join_entities(species_list)
if not species:
species = ""

phenotype = instance.phenotype.phenotype_str if instance.phenotype else ''
origin_names = [origin.name for origin in instance.origins.all()]
origins = join_entities(origin_names)
if not origins:
origins = ""

circuit_type = instance.get_circuit_type_display() if instance.circuit_type else None
projection = instance.get_projection_display() if instance.projection else None
projection_phenotype = str(instance.projection_phenotype) if instance.projection_phenotype else ''

laterality_description = instance.get_laterality_description()

apinatomy = instance.apinatomy_model if instance.apinatomy_model else ""
journey_sentence = '; '.join(journey)

# Creating the statement
if sex or species != "":
statement = f"In {sex or ''} {species}, the {phenotype.lower()} connection goes {journey_sentence}.\n"
else:
statement = f"A {phenotype.lower()} connection goes {journey_sentence}.\n"

statement += f"This "
if projection:
statement += f"{projection.lower()} "
if projection_phenotype:
statement += f"{projection_phenotype.lower()} "
if circuit_type:
statement += f"{circuit_type.lower()} "

statement += f"connection projects from the {origins}."
if laterality_description:
statement = statement[:-1] + f" and is found {laterality_description}.\n"

if apinatomy:
statement += f" It is described in {apinatomy} model."

return statement.strip().replace(" ", " ")
return create_statement_preview(instance, self.context['journey'])

def get_errors(self, instance) -> List:
return get_connectivity_errors(instance)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
# Generated by Django 4.1.4 on 2024-12-18 15:59

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
("composer", "0067_auto_20241218_0928"),
]

operations = [
migrations.AddField(
model_name="connectivitystatement",
name="statement_prefix",
field=models.TextField(blank=True, null=True),
),
migrations.AddField(
model_name="connectivitystatement",
name="statement_suffix",
field=models.TextField(blank=True, null=True),
),
]
24 changes: 24 additions & 0 deletions backend/composer/migrations/0069_auto_20241218_1659.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# Generated by Django 4.1.4 on 2024-12-18 15:59

from composer.services.statement_service import get_prefix_for_statement_preview, get_suffix_for_statement_preview
from django.db import migrations


def update_suffix_prefix_for_connectivity_statement_fields(apps, schema_editor):
ConnectivityStatement = apps.get_model('composer', 'ConnectivityStatement')
for cs in ConnectivityStatement.objects.all():
cs.statement_prefix = get_prefix_for_statement_preview(cs)
cs.statement_suffix = get_suffix_for_statement_preview(cs)
cs.save(update_fields=["statement_prefix", "statement_suffix"])


class Migration(migrations.Migration):

dependencies = [
("composer", "0068_connectivitystatement_statement_prefix_and_more"),
]

operations = [
migrations.RunPython(
update_suffix_prefix_for_connectivity_statement_fields)
]
8 changes: 3 additions & 5 deletions backend/composer/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -191,10 +191,6 @@ class Sex(models.Model):
def __str__(self):
return self.name

@property
def sex_str(self):
return str(self.name) if self.name else ''

class Meta:
ordering = ["name"]
verbose_name_plural = "Sex"
Expand Down Expand Up @@ -568,6 +564,8 @@ class ConnectivityStatement(models.Model):
created_date = models.DateTimeField(auto_now_add=True, db_index=True)
modified_date = models.DateTimeField(auto_now=True, db_index=True)
journey_path = models.JSONField(null=True, blank=True)
statement_prefix = models.TextField(null=True, blank=True)
statement_suffix = models.TextField(null=True, blank=True)

def __str__(self):
suffix = ""
Expand Down Expand Up @@ -1030,7 +1028,7 @@ class AlertType(models.Model):

def __str__(self):
return self.name


class StatementAlert(models.Model):
connectivity_statement = models.ForeignKey(
Expand Down
11 changes: 8 additions & 3 deletions backend/composer/services/export_services.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
get_complete_from_entities_for_destination,
get_complete_from_entities_for_via,
)
from ..services.statement_service import create_statement_preview
from composer.services.filesystem_service import create_dir_if_not_exists
from composer.services.state_services import ConnectivityStatementStateService

Expand Down Expand Up @@ -106,8 +107,12 @@ def get_statement_uri(cs: ConnectivityStatement, row: Row):
return cs.reference_uri


def get_alert_text(cs: ConnectivityStatement, row: Row):
return row.object_text__from_alert_notes
def get_object_text(cs: ConnectivityStatement, row: Row):
# object text can be either from - alert notes or statement preview
if row.object_text__from_alert_notes:
afonsobspinto marked this conversation as resolved.
Show resolved Hide resolved
return row.object_text__from_alert_notes
else:
return create_statement_preview(cs, cs.get_journey())

def get_nlp_id(cs: ConnectivityStatement, row: Row):
return cs.export_id
Expand Down Expand Up @@ -206,7 +211,7 @@ def generate_csv_attributes_mapping() -> Dict[str, Callable]:
"Predicate Relationship": get_relationship,
"Object": get_structure,
"Object URI": get_identifier,
"Object Text": get_alert_text,
"Object Text": get_object_text,
"Axonal course poset": get_layer,
"Connected from": get_connected_from_names,
"Connected from uri": get_connected_from_uri,
Expand Down
73 changes: 73 additions & 0 deletions backend/composer/services/statement_service.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
from ..utils import join_entities
from django.apps import apps


def get_prefix_for_statement_preview(cs) -> str:
connectivity_statement_obj = apps.get_model(
'composer', 'ConnectivityStatement')
connectivity_statement = connectivity_statement_obj.objects.get(
Copy link
Member

Choose a reason for hiding this comment

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

question: Isn't cs the connectivity statement instance already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here yes all the fields we want can be accessed using the cs instance, and hence I updated the code to get it directly from cs instance. thanks for pointing this out!

id=cs.id)

sex = connectivity_statement.sex.name if connectivity_statement.sex else None

species_list = [
specie.name for specie in connectivity_statement.species.all()]
species = join_entities(species_list)
if not species:
species = ""

phenotype = connectivity_statement.phenotype.name if connectivity_statement.phenotype else ''
if sex or species != "":
statement = f"In {sex or ''} {species}, the {phenotype.lower()} connection goes"
else:
statement = f"A {phenotype.lower()} connection goes"
return statement


def get_suffix_for_statement_preview(cs):
connectivity_statement_obj = apps.get_model(
'composer', 'ConnectivityStatement')
connectivity_statement = connectivity_statement_obj.objects.get(
Copy link
Member

Choose a reason for hiding this comment

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

Same question as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is needed here yes @afonsobspinto, because we need the origin.name and get_laterality_description(), both not accessible with the cs instance. However we can get them using connectivity_statement object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a similar case as - #381 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

If we have the time, my preference would be to add the necessary methods inline on the migration (or in a migrations helper file). The reasoning being that I don't think that makes sense to to harm the performance of the server that's running all the time, for ease of use in one time, the migration; Besides I would say conceptually it makes more sense to do so, so that the migration is independent of the model evolution over time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you for the suggestion @afonsobspinto :)

id=cs.id)

circuit_type = connectivity_statement.get_circuit_type_display(
) if connectivity_statement.circuit_type else None
projection = connectivity_statement.get_projection_display(
) if connectivity_statement.projection else None
projection_phenotype = str(
connectivity_statement.projection_phenotype) if connectivity_statement.projection_phenotype else ''

laterality_description = connectivity_statement.get_laterality_description()
apinatomy = connectivity_statement.apinatomy_model if connectivity_statement.apinatomy_model else ""

origin_names = [
origin.name for origin in connectivity_statement.origins.all()]
origins = join_entities(origin_names)

if not origins:
origins = ""

statement = f"This "
if projection:
statement += f"{projection.lower()} "
if projection_phenotype:
statement += f"{projection_phenotype.lower()} "
if circuit_type:
statement += f"{circuit_type.lower()} "

statement += f"connection projects from the {origins}."
if laterality_description:
statement = statement[:-1] + \
f" and is found {laterality_description}.\n"

if apinatomy:
statement += f" It is described in {apinatomy} model."
return statement


def create_statement_preview(cs, journey):
prefix = cs.statement_prefix
journey_sentence = '; '.join(journey)
suffix = cs.statement_suffix
statement = f'{prefix} {journey_sentence}.\n{suffix}'
return statement.strip().replace(" ", " ")
33 changes: 28 additions & 5 deletions backend/composer/signals.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from django_fsm.signals import post_transition

from composer.services.layers_service import update_from_entities_on_deletion
from composer.services.statement_service import get_suffix_for_statement_preview, get_prefix_for_statement_preview
from .utils import update_modified_date
from .enums import CSState, NoteType
from .models import (
Expand All @@ -17,12 +18,12 @@
Layer,
Region,
Via,
Specie,
)
from .services.export_services import compute_metrics, ConnectivityStatementStateService
from .services.graph_service import recompile_journey_path



@receiver(post_save, sender=ExportBatch)
def export_batch_post_save(sender, instance=None, created=False, **kwargs):
if created and instance:
Expand Down Expand Up @@ -105,7 +106,6 @@ def connectivity_statement_origins_changed(sender, instance, action, pk_set, **k
pass
recompile_journey_path(instance)


# Call `update_from_entities_on_deletion` for each removed entity
if action == "post_remove" and pk_set:
for deleted_entity_id in pk_set:
Expand Down Expand Up @@ -143,7 +143,6 @@ def via_from_entities_changed(sender, instance, action, **kwargs):
recompile_journey_path(instance.connectivity_statement)



# Signals for Destination anatomical_entities
@receiver(m2m_changed, sender=Destination.anatomical_entities.through)
def destination_anatomical_entities_changed(sender, instance, action, **kwargs):
Expand All @@ -157,7 +156,6 @@ def destination_anatomical_entities_changed(sender, instance, action, **kwargs):
recompile_journey_path(instance.connectivity_statement)



# Signals for Destination from_entities
@receiver(m2m_changed, sender=Destination.from_entities.through)
def destination_from_entities_changed(sender, instance, action, **kwargs):
Expand All @@ -171,7 +169,6 @@ def destination_from_entities_changed(sender, instance, action, **kwargs):
recompile_journey_path(instance.connectivity_statement)



# Signals for Via model changes
@receiver(post_save, sender=Via)
@receiver(post_delete, sender=Via)
Expand Down Expand Up @@ -210,3 +207,29 @@ def note_post_save_and_delete(sender, instance, **kwargs):
update_modified_date(instance.sentence)
if instance.connectivity_statement:
update_modified_date(instance.connectivity_statement)


@receiver(m2m_changed, sender=ConnectivityStatement.species.through, dispatch_uid="update_prefix_suffix_for_connectivity_statement_preview")
@receiver(m2m_changed, sender=ConnectivityStatement.origins.through, dispatch_uid="update_prefix_suffix_for_connectivity_statement_preview")
@receiver([post_save, post_delete], sender=ConnectivityStatement, dispatch_uid="update_prefix_suffix_for_connectivity_statement_preview")
def update_prefix_suffix_for_connectivity_statement_preview(sender, instance, action=None, **kwargs):
if isinstance(instance, ConnectivityStatement):
Copy link
Member

@afonsobspinto afonsobspinto Jan 17, 2025

Choose a reason for hiding this comment

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

Here's a good case where we can invert the condition. Instead of having all the logic wrapped in a if we can do:

    if not isinstance(instance, ConnectivityStatement):
        return
    *Function logic*

relevant_fields_for_suffix = ['circuit_type', 'projection',
'projection_phenotype_id', 'laterality', 'apinatomy_model']

relevant_fields_for_prefix = ['sex', 'phenotype']
relevant_fields = relevant_fields_for_suffix + relevant_fields_for_prefix
updated_fields = kwargs.get('update_fields', [])
updated_fields = updated_fields if updated_fields else []

has_species_or_origin_changed = action in [
"post_add", "post_remove", "post_clear"]
has_relevant_field_changed = any(
[field in updated_fields for field in relevant_fields])
if has_species_or_origin_changed or has_relevant_field_changed:
instance.statement_prefix = get_prefix_for_statement_preview(
instance)
instance.statement_suffix = get_suffix_for_statement_preview(
instance)
instance.save(
update_fields=["statement_prefix", "statement_suffix"])
Loading