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

AJP-3 introduce AI predictions #136

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

angrun
Copy link
Collaborator

@angrun angrun commented Oct 14, 2023

Screenshot 2023-10-14 at 18 53 24

Copy link

sonarqubecloud bot commented Nov 4, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 1 Security Hotspot
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

Copy link

codecov bot commented Dec 8, 2023

Codecov Report

Attention: 139 lines in your changes are missing coverage. Please review.

Comparison is base (87af58c) 15.55% compared to head (2789323) 15.65%.
Report is 66 commits behind head on master.

Files Patch % Lines
ajapaik/ajapaik/models.py 40.00% 42 Missing ⚠️
ajapaik/ajapaik/fotis_utils.py 0.00% 36 Missing ⚠️
ajapaik/ajapaik/curator_drivers/fotis.py 0.00% 30 Missing ⚠️
...aik/management/commands/import_photos_from_muis.py 0.00% 12 Missing ⚠️
ajapaik/ajapaik/curator_drivers/finna.py 0.00% 8 Missing ⚠️
ajapaik/ajapaik/forms.py 0.00% 8 Missing ⚠️
...japaik/ajapaik/curator_drivers/wikimediacommons.py 0.00% 2 Missing ⚠️
ajapaik/ajapaik/curator_drivers/common.py 0.00% 1 Missing ⚠️
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     
Flag Coverage Δ
unittests 15.65% <24.04%> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

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? :)

Copy link
Collaborator Author

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)
Copy link
Member

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()'?



class PhotoModelSuggestionAlternativeCategory(Suggestion):
INTERIOR, EXTERIOR = range(2)
Copy link
Member

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed!

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)
Copy link
Member

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

on_delete=CASCADE)

def validate_unique(self, exclude=None):
# super().validate_unique(exclude)
Copy link
Member

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 :)

Copy link
Member

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?

Copy link
Collaborator Author

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.

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions

1 Security Hotspot

See analysis details on SonarCloud

@angrun angrun requested a review from MaertHaekkinen January 2, 2024 19:13
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()
Copy link
Member

Choose a reason for hiding this comment

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

we could just

Suggested change
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
Copy link
Member

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") {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (viewpointElevationVerdict === "raised") {
if (viewpointElevationVerdict === "aerial") {

if (fields["scene"] === 0) {
modelCategory["scene"] = "interior";
} else {
modelCategory["scene"] = "exterior";
Copy link
Member

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?

@MaertHaekkinen
Copy link
Member

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)

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.

2 participants