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

Adding degree and first_timer stats #303

Merged
merged 36 commits into from
Apr 9, 2020

Conversation

raquelpanapalen
Copy link
Contributor

@raquelpanapalen raquelpanapalen commented Mar 25, 2020

What this PR does / Why we need it

Adding degree stats (only shown the top 10)
Adding first_timer stats
Adding lennyface stats (top 5)
Adding resume stats
Optimization
Defaultdict(int)

Which issue(s) this PR fixes (optional)

References #103

Special notes for your reviewer (optional)

Some questions

  • I have read the contributing guidelines
  • I abide by this repository Code of Conduct
  • I understand that my PR won't be merged until GitHub gives a "green light"

Additional Notes (optional)

Do you want to add anything else? We ❤️ to hear your opinions!

stats/views.py Outdated
degree_count_confirmed = Application.objects.filter(status=APP_CONFIRMED).values('degree') \
.annotate(applications=Count('degree')) \
.order_by('-applications')[:10]

Copy link
Member

Choose a reason for hiding this comment

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

This is starting to have too many queries that look like the same. Any chance we can factor all those into a single query? I'm afraid on performance here and pushing too hard on the DB.

Copy link
Member

@Casassarnau Casassarnau Mar 26, 2020

Choose a reason for hiding this comment

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

Query sets are lazy, ones you call them they call the DB. If we put it in one "query" it really doesn't matter, django will call them one by one. Or at least it's what I understant from the documentation.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but you have several query sets here, which translates to several DB queries on the background. Making one query that returns degrees/university/food... and then processing the dictionary to generate the aggregations may actually perform better as it runs in a single query and reduces the amount of times it requires to connect to the DB.

type: 'category'
}
}
});
Copy link
Member

Choose a reason for hiding this comment

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

Same for the UI? Maybe it's time to create a new tab? It may become unreadable.

@raquelpanapalen raquelpanapalen changed the title Adding degree stats Adding degree and first_timer stats Apr 1, 2020
Copy link
Member

@casassg casassg left a comment

Choose a reason for hiding this comment

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

Linter is failing:

./stats/views.py:97:5: E303 too many blank lines (2)

Apart from that and a small improvement, lgtm

stats/views.py Outdated Show resolved Hide resolved
@casassg casassg merged commit 5eee3ad into HackAssistant:master Apr 9, 2020
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.

3 participants