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

Add consistency to participation in /contest #1743

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

Conversation

jtyliu
Copy link
Contributor

@jtyliu jtyliu commented Aug 4, 2021

TODOs
/contest

  • old_volatility
  • new_volatility
    [ ] virtual_participation_number (Always 0)

/participations

  • old_rating
  • new_rating
  • old_volatility
  • new_volatility
    - [ ] solutions (Out of context if placed in api)

/user
- [ ] user ? (Redundant)

  • start_time
  • end_time
  • tiebreaker
  • old_rating
  • new_rating
  • is_disqualified
    - [ ] solutions (Out of context)
  • old_volatility
  • new_volatility
    - [ ] virtual_participation_number (Always 0)

Also, a fix needs to be implemented for #1742 or else the same error will propagate to these proposed changes and cause confusion

Perhaps increase mccc15s by a microsecond

@codecov-commenter
Copy link

codecov-commenter commented Aug 10, 2021

Codecov Report

Merging #1743 (f2cd2cd) into master (bf3fbaf) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1743      +/-   ##
==========================================
- Coverage   46.16%   46.16%   -0.01%     
==========================================
  Files         223      223              
  Lines       12658    12659       +1     
==========================================
  Hits         5844     5844              
- Misses       6814     6815       +1     
Impacted Files Coverage Δ
judge/views/api/api_v2.py 47.60% <0.00%> (-0.20%) ⬇️
judge/admin/runtime.py 66.66% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf3fbaf...f2cd2cd. Read the comment docs.

@jtyliu jtyliu marked this pull request as ready for review August 10, 2021 22:52
Copy link
Member

@Ninjaclasher Ninjaclasher left a comment

Choose a reason for hiding this comment

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

Can you also create a PR to https://github.com/DMOJ/docs?

judge/views/api/api_v2.py Show resolved Hide resolved
@@ -536,17 +568,27 @@ def get_object_data(self, profile):
contest__in=Contest.get_visible_contests(self.request.user),
contest__end_time__lt=self._now,
)
.annotate(
old_rating=Subquery(old_ratings_subquery.values('rating')[:1]),
new_rating=Subquery(new_ratings_subquery.values('rating')[:1]),
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this gets optimized properly, but why not just use rating__rating and rating__volatility instead of a subquery here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Yes

judge/views/api/api_v2.py Show resolved Hide resolved
@Ninjaclasher
Copy link
Member

Also, perhaps we should hold off on this PR until #1751 .

Copy link
Member

@Ninjaclasher Ninjaclasher left a comment

Choose a reason for hiding this comment

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

LGTM, though please create a PR to https://github.com/DMOJ/docs/.

jtyliu pushed a commit to jtyliu/docs that referenced this pull request Aug 26, 2021
judge/views/api/api_v2.py Outdated Show resolved Hide resolved
Copy link
Member

@Ninjaclasher Ninjaclasher left a comment

Choose a reason for hiding this comment

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

LGTM. @int-y1 when you have time, could you take another look?

Copy link
Contributor

@int-y1 int-y1 left a comment

Choose a reason for hiding this comment

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

i don't think this pr was tested

new_rating=F('rating_rating'),
old_mean=Subquery(old_ratings_subquery.values('mean')[:1]),
new_mean=F('rating_mean'),
performance=F('rating_performance'),
Copy link
Contributor

Choose a reason for hiding this comment

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

should use rating__
this affects all occurrences of rating_rating, rating_mean, rating_performance in this pr

'new_rating',
'old_mean',
'new_mean',
'performance',
Copy link
Contributor

Choose a reason for hiding this comment

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

.only doesn't apply to annotations (and this causes a 500)

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.

None yet

4 participants