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

Change the total number of submissions to the number of submissions p… #2722

Merged
merged 1 commit into from
Sep 29, 2024

Conversation

as6325400
Copy link
Contributor

Tracking issue

issue

Description

Change the cumulative number of submissions over time to the number of submissions per time interval.

Screenshots

截圖 2024-09-26 下午1 00 47

@as6325400
Copy link
Contributor Author

I'm not very familiar with Twig, but I believe there would be a more efficient approach in terms of time complexity if implemented in the controller. However, given the scale of the competition's data, the difference may not be significant.

@as6325400
Copy link
Contributor Author

Optimize time complexity

2000 submissions time (ms)
Origin 9859 ms
Optimize 277 ms

原始執行時間
9859 ms

優化後執行時間
277 ms

@meisterT
Copy link
Member

This is from this year's finals:
image

I wonder how this would look with log scale (I tried making that work but didn't succeed, perhaps someone else has an idea).

There is another minor thing we might want to improve: in case the contest duration is not just 5 hours, but say 1 week or even more we create too many data points. We could decide to always use a fixed number of buckets, e.g. 60 - in a 5h contest this would group things together in 5min buckets. That should work well for most contest durations.

@meisterT meisterT requested a review from eldering September 27, 2024 16:50
@as6325400
Copy link
Contributor Author

This is from this year's finals: image

I wonder how this would look with log scale (I tried making that work but didn't succeed, perhaps someone else has an idea).

There is another minor thing we might want to improve: in case the contest duration is not just 5 hours, but say 1 week or even more we create too many data points. We could decide to always use a fixed number of buckets, e.g. 60 - in a 5h contest this would group things together in 5min buckets. That should work well for most contest durations.

I think the number of buckets can be fixed, and it's not difficult to change this. Should we make this change in this PR itself? Or should we discuss it later? I feel that 60 might be a bit too few.

@mpsijm
Copy link
Contributor

mpsijm commented Sep 27, 2024

If it's going to be buckets (whether they're gonna be 1 minute or 5 minutes or some other size), maybe a bar chart would work better than a line graph? There's no continuity between each subsequent pair of buckets 🙂

@as6325400
Copy link
Contributor Author

If it's going to be buckets (whether they're gonna be 1 minute or 5 minutes or some other size), maybe a bar chart would work better than a line graph? There's no continuity between each subsequent pair of buckets 🙂

I think this is a good approach, although in very dense situations, these two charts might look quite similar.

@meisterT
Copy link
Member

I'm fine with doing the bucketing in a separate PR as this is already an improvement.

I also wondered what a (stacked) bar chart might look like. Do you want to give this a try now or later?

@as6325400
Copy link
Contributor Author

I'm fine with doing the bucketing in a separate PR as this is already an improvement.

I also wondered what a (stacked) bar chart might look like. Do you want to give this a try now or later?

Since I'm not sure if the bar chart would be better, I'll implement the bar chart version in another branch and then submit a PR for you to compare the differences. Does that work for you?

@meisterT
Copy link
Member

👍

}, {});

submissions.forEach(submission => {
let submission_minute = Math.round((submission.submittime - submission.starttime) / 60);
Copy link
Member

Choose a reason for hiding this comment

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

Small detail: this should not be rounded, but the floor taken. A submission at 57 seconcs into the contest is in minute 0, not minute 1. Above contest_duration_minutes should be taken the ceil though, to make sure that 0 <= submission_minute < contest_duration_minutes always holds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If a contest lasts 300 minutes, can a submission be made at 299 minutes and 59 seconds? Is it possible to submit during the 300th minute? If not, then should contest_duration_minutes be taken as the floor (rounded down)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, sorry, I understand now. It was my mistake.

@@ -388,7 +409,7 @@ nv.addGraph(function() {
.showYAxis(true) //Show the y-axis
.showXAxis(true) //Show the x-axis
.forceX([0, {{ (current_contest.endtime - current_contest.starttime) / 60 }}])
.forceY([0, {{ submissions|length *1.10 }}])
.forceY([0, {{ max_submissions_per_minute * 1.10 }}])
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I overlook something, but where is max_submissions_per_minute ever updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my oversight. I've fixed it.

@eldering
Copy link
Member

I'm fine with doing the bucketing in a separate PR as this is already an improvement.

I also wondered what a (stacked) bar chart might look like. Do you want to give this a try now or later?

I think a bar chart would be clearer, but I'm fine with doing that in a second iteration.

@as6325400
Copy link
Contributor Author

Do I need to squash them to a single commit?

@eldering
Copy link
Member

Do I need to squash them to a single commit?

Preferably yes, at least for bugfixes. You can do that either locally with git rebase --interactive or if there's really only one single logical commit necessary (which I think makes sense here), then tick the box in github.

…er minute.

Beta

Optimize time complexity.

fix count miniute edge bug
@meisterT meisterT added this pull request to the merge queue Sep 29, 2024
Merged via the queue into DOMjudge:main with commit bba7850 Sep 29, 2024
26 checks passed
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.

4 participants