-
Notifications
You must be signed in to change notification settings - Fork 19
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
AJP-3 introduce AI predictions #136
base: master
Are you sure you want to change the base?
Conversation
angrun
commented
Oct 14, 2023
•
edited
Loading
edited

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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that these are changes that were needed for local environment - not something that we want to merge to master? :)
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.
Sure, correct!
|
||
if result.exists(): | ||
categories = serializers.serialize('python', result) | ||
result_categories.append(categories) |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
Addressed!
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (viewpointElevationVerdict === "raised") { | |
if (viewpointElevationVerdict === "aerial") { |
if (fields["scene"] === 0) { | ||
modelCategory["scene"] = "interior"; | ||
} else { | ||
modelCategory["scene"] = "exterior"; |
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.
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) |