-
Notifications
You must be signed in to change notification settings - Fork 0
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-324 - journey computation #381
base: develop
Are you sure you want to change the base?
Changes from 12 commits
ddd7d9c
22ba03f
ea1297f
5b03c6d
ae6bcb0
01436c0
b8a1d9e
50978d1
51b3838
b934de3
99d1238
466d8b3
cc18966
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
# Generated by Django 4.1.4 on 2024-12-18 08:25 | ||
|
||
from django.db import migrations, models | ||
|
||
|
||
class Migration(migrations.Migration): | ||
|
||
dependencies = [ | ||
("composer", "0065_alter_statementalert_text"), | ||
] | ||
|
||
operations = [ | ||
migrations.AddField( | ||
model_name="connectivitystatement", | ||
name="journey_path", | ||
field=models.JSONField(blank=True, null=True), | ||
), | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
# Generated by Django 4.1.4 on 2024-12-18 08:28 | ||
|
||
from django.db import migrations | ||
from django.db import migrations | ||
from composer.services.graph_service import compile_journey | ||
|
||
|
||
def update_journey_fields(apps, schema_editor): | ||
ConnectivityStatement = apps.get_model('composer', 'ConnectivityStatement') | ||
for cs in ConnectivityStatement.objects.all(): | ||
cs.journey_path = compile_journey(cs) | ||
cs.save(update_fields=["journey_path"]) | ||
|
||
|
||
class Migration(migrations.Migration): | ||
|
||
dependencies = [ | ||
("composer", "0066_connectivitystatement_journey_path"), | ||
] | ||
|
||
operations = [ | ||
migrations.RunPython(update_journey_fields), | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -88,9 +88,7 @@ def consolidate_paths(paths): | |
|
||
paths = consolidated + [paths[i] for i in range(len(paths)) if i not in used_indices] | ||
|
||
return paths, [[((node[1].replace(JOURNEY_DELIMITER, ' or '), node[2]) if ( | ||
node[2] == 0 or path.index(node) == len(path) - 1) else ( | ||
node[1].replace(JOURNEY_DELIMITER, ', '), node[2])) for node in path] for path in paths] | ||
return paths | ||
|
||
|
||
def can_merge(path1, path2): | ||
|
@@ -144,7 +142,7 @@ def merge_paths(path1, path2): | |
return merged_path | ||
|
||
|
||
def compile_journey(connectivity_statement) -> dict: | ||
def compile_journey(connectivity_statement) -> List[str]: | ||
""" | ||
Generates a string of descriptions of journey paths for a given connectivity statement. | ||
|
||
|
@@ -157,17 +155,55 @@ def compile_journey(connectivity_statement) -> dict: | |
# Extract origins, vias, and destinations from the connectivity statement | ||
Via = apps.get_model('composer', 'Via') | ||
Destination = apps.get_model('composer', 'Destination') | ||
Origin = apps.get_model('composer', 'AnatomicalEntity') | ||
|
||
origins = list(connectivity_statement.origins.all()) | ||
|
||
vias = list(Via.objects.filter(connectivity_statement=connectivity_statement)) | ||
destinations = list(Destination.objects.filter(connectivity_statement=connectivity_statement)) | ||
vias = list(Via.objects.filter(connectivity_statement__id=connectivity_statement.id)) | ||
destinations = list(Destination.objects.filter(connectivity_statement__id=connectivity_statement.id)) | ||
origins = list(Origin.objects.filter(origins_relations__id=connectivity_statement.id).distinct()) | ||
# origins = list(connectivity_statement.origins.all()) | ||
|
||
# Generate all paths and then consolidate them | ||
all_paths2 = generate_paths(origins, vias, destinations) | ||
consolidated_paths, journey_paths = consolidate_paths(all_paths2) | ||
consolidated_paths = consolidate_paths(all_paths2) | ||
return consolidated_paths | ||
|
||
|
||
def get_journey_path_from_consolidated_paths(consolidated_paths): | ||
journey_paths = [[((node[1].replace(JOURNEY_DELIMITER, ' or '), node[2]) if ( | ||
node[2] == 0 or path.index(node) == len(path) - 1) else ( | ||
node[1].replace(JOURNEY_DELIMITER, ', '), node[2])) for node in path] for path in consolidated_paths] | ||
return journey_paths | ||
|
||
|
||
def build_journey_description(consolidated_paths, connectivity_statement): | ||
journey_paths = get_journey_path_from_consolidated_paths( | ||
consolidated_paths) | ||
Via = apps.get_model('composer', 'Via') | ||
vias = list(Via.objects.filter( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: Why do we need to fetch the vias here? can't we rely entirely on the new journey paths attribute created? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand what you mean yes, this is only to get the len(vias) later with - here is the journey_paths and in this case, len(vias from the CS) is 1. where the value of that via is:
I am unable to see the pattern to relate them. Do you think there is one @afonsobspinto that I might be missing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I understand the code correctly we don't need the vias count; we can just do something on the lines of:
which identifies the vias by looking for nodes with layers between origin (0) and destination (highest layer) |
||
connectivity_statement=connectivity_statement)) | ||
|
||
# Create sentences for each journey path | ||
journey_descriptions = [] | ||
for path in journey_paths: | ||
origin_names = path[0][0] | ||
destination_names = path[-1][0] | ||
via_names = ' via '.join([node for node, layer in path if 0 < layer < len(vias) + 1]) | ||
|
||
if via_names: | ||
sentence = f"from {origin_names} to {destination_names} via {via_names}" | ||
else: | ||
sentence = f"from {origin_names} to {destination_names}" | ||
|
||
journey_descriptions.append(sentence) | ||
return journey_descriptions | ||
|
||
|
||
def build_journey_entities(consolidated_paths, connectivity_statement): | ||
entities = [] | ||
Via = apps.get_model('composer', 'Via') | ||
vias = list(Via.objects.filter( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
connectivity_statement=connectivity_statement)) | ||
|
||
for path in consolidated_paths: | ||
origin_splits = path[0][0].split(JOURNEY_DELIMITER) | ||
destination_splits = path[-1][0].split(JOURNEY_DELIMITER) | ||
|
@@ -181,22 +217,9 @@ def compile_journey(connectivity_statement) -> dict: | |
'vias': [{'label': node, 'id': node_id} for node_id, node, layer in path if 0 < layer < len(vias) + 1] | ||
} | ||
entities.append(entity) | ||
return entities | ||
|
||
# Create sentences for each journey path | ||
journey_descriptions = [] | ||
for path in journey_paths: | ||
origin_names = path[0][0] | ||
destination_names = path[-1][0] | ||
via_names = ' via '.join([node for node, layer in path if 0 < layer < len(vias) + 1]) | ||
|
||
if via_names: | ||
sentence = f"from {origin_names} to {destination_names} via {via_names}" | ||
else: | ||
sentence = f"from {origin_names} to {destination_names}" | ||
|
||
journey_descriptions.append(sentence) | ||
|
||
return { | ||
'journey': journey_descriptions, | ||
'entities': entities | ||
} | ||
def recompile_journey_path(instance): | ||
instance.journey_path = compile_journey(instance) | ||
instance.save(update_fields=["journey_path"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Why did we change from:
origins = list(connectivity_statement.origins.all())
to
origins = list(Origin.objects.filter(origins_relations__id=connectivity_statement.id).distinct())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reason is here in the code - https://github.com/MetaCell/sckan-composer/blob/feature/SCKAN-324/backend/composer/services/graph_service.py#L24
It says:
so when generating the path - we do have the origins, but not the origin.name - which is a property in AnatomicalEntity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@afonsobspinto did you do the check with the API call?
This happens only when we do the migration - possibly something with -
apps.get_model('composer', 'ConnectivityStatement')
in the migration file (ref - 0067 auto migration file). Also if you notice this is only for the@properties
and not for the fields.So if you put a breakpoint to the above mentioned code, and then run the migration to db - this should cause the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood, during migrations django uses historical models that don't include python level constructs like @Property.
I think the new instruction works, but I would remove the .distinct(), I don't think it's needed in our context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before removing it, I want to confirm again with you if you are okay to remove the
distinct()
here, the reason I had that is because unlike vias and destinations - which are FK, origins is a M2M.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ManyToManyField in Django automatically ensures that the relationship between two models is unique. In other words, we know that we have a unique pair of ConnectivityStatement and AnatomicalEntity. Here's a simple test:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With that said, if we have the time I would apply the same suggestion I mentioned here:
#404 (comment)