AJP-3 introduce AI predictions #136
Conversation
angrun
commented
Oct 14, 2023
•
angrun
commented
|
SonarCloud Quality Gate failed.
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #136 +/- ##
==========================================
+ Coverage 15.55% 15.65% +0.10%
==========================================
Files 164 168 +4
Lines 11186 11330 +144
==========================================
+ Hits 1740 1774 +34
- Misses 9446 9556 +110
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
ajapaik/settings/default.py
Outdated
There was a problem hiding this comment.
I guess that these are changes that were needed for local environment - not something that we want to merge to master? :)
|
|
||
| if result.exists(): | ||
| categories = serializers.serialize('python', result) | ||
| result_categories.append(categories) |
There was a problem hiding this comment.
What's the point of appending, if we return on the next line? I think that we could just "return categories", and skip appending?
Can even remove result_categories and just 'return []' as a fallback in case not 'result.exists()'?
ajapaik/ajapaik/models.py
Outdated
|
|
||
|
|
||
| class PhotoModelSuggestionAlternativeCategory(Suggestion): | ||
| INTERIOR, EXTERIOR = range(2) |
There was a problem hiding this comment.
I think we should not duplicate
INTERIOR, EXTERIOR
....
....
VIEWPOINT_ELEVATION_CHOICES
Variables, as they are all the same. We can just define them outside of Class, on top of this file? In case we have any previous usages of the same variables, could remove those usages as well
ajapaik/ajapaik/models.py
Outdated
| viewpoint_elevation_alternation = PositiveSmallIntegerField(_('Viewpoint elevation'), | ||
| choices=VIEWPOINT_ELEVATION_CHOICES, blank=True, | ||
| null=True) | ||
| scene_alternation = PositiveSmallIntegerField(_('Scene'), choices=SCENE_CHOICES, blank=True, null=True) |
There was a problem hiding this comment.
I feel like the _alternation suffix could be skipped here, as the class/model name already implies that this value is an alternate.
Same for viewpoint_elevation_alternation
ajapaik/ajapaik/models.py
Outdated
| on_delete=CASCADE) | ||
|
|
||
| def validate_unique(self, exclude=None): | ||
| # super().validate_unique(exclude) |
There was a problem hiding this comment.
I think that uniqueness in Django can be achieved by using
https://docs.djangoproject.com/en/3.2/ref/models/constraints/#django.db.models.UniqueConstraint
So that we don't need to write a custom validate_unique method.
Right now it's unique if:
scene_alternation is null/blank OR photo, proposer are unique together with scene_alternation?
It might not work here, but just thought to mention just in case :)
There was a problem hiding this comment.
So, I'm just checking if uniqueness logic is this specific that we don't care if scene_alternation is null/blank?
There was a problem hiding this comment.
Yes, you are correct here. Indeed viewpoint elevation was missing. I addressed the comment accordingly.
|
| result.viewpoint_elevation = data.get("verdict_view_point_elevation", None) | ||
| result.scene = data.get("verdict_scene", None) | ||
| result.photo_id = data.get("photo_id", None) | ||
| result.save() |
There was a problem hiding this comment.
we could just
| result.save() | |
| PhotoModelSuggestionResult.objects.create( | |
| viewpoint_elevation = data.get("verdict_view_point_elevation", None) | |
| scene = data.get("verdict_scene", None) | |
| photo_id = data.get("photo_id", None) | |
| ) |
and remove all the result. assignment + result = ` statements.
Do we want to support nulls in PhotoModelSuggestionResult, especially for photo? I guess some photos might get deleted, but in that case we could cascade delete the suggestions, as there is no way to tell for what this suggestion was.
I think that photo should not be nullable, and scene OR viewpoint_elevation should be required - we can not achieve this in the model AFAIK, but we could have it in the validation.
| payload["viewpoint_elevation_to_alternate"] = 1 | ||
| } | ||
| if (viewpointElevationVerdict === "raised") { | ||
| payload["viewpoint_elevation_to_alternate"] = 2 |
There was a problem hiding this comment.
Just to clarify, do we need to check for === "raised" twice? In any case, that would mean the value is always 2 - if it is raised.
I guess there is another viewpointElevationVerdict - "aerial" (or something of the sort)?
| if (viewpointElevationVerdict === "raised") { | ||
| payload["viewpoint_elevation_to_alternate"] = 1 | ||
| } | ||
| if (viewpointElevationVerdict === "raised") { |
There was a problem hiding this comment.
| if (viewpointElevationVerdict === "raised") { | |
| if (viewpointElevationVerdict === "aerial") { |
| if (fields["scene"] === 0) { | ||
| modelCategory["scene"] = "interior"; | ||
| } else { | ||
| modelCategory["scene"] = "exterior"; |
There was a problem hiding this comment.
Usually we have used else if - is there a risk that scene can be null, but we handle it as exterior or do we need a fallback value (exterior) here?
|
I am sorry for the late replies - I thought that my comments were submitted - but I am not used to Github review (I use Gitlab usually) |
