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

Added trajectory table to data page #66

Merged
merged 4 commits into from
Sep 27, 2023
Merged

Conversation

achasmita
Copy link
Contributor

No description provided.

Copy link
Contributor

@shankari shankari left a comment

Choose a reason for hiding this comment

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

I think that this is generally fine, which is not surprising since it is essentially a copy of the demographic changes.

However:

  • since we have already missed the previous release train, and
  • the trajectory data is many orders of magnitude larger than the trip data, so loading it is likely to be slow

While we can and will address this in a more principled fashion in
#55
I also don't want to run the risk of a regression by making the admin dashboard unusable with this change.

So, I think we should, at least temporarily, change the loading pattern for this table. Instead of loading the data when the admin dashboard is launched, we should load the data only when the user accesses the "trajectories" tab.

That will keep the admin dashboard performant overall, and reserve the performance hit for if/when the admin actually clicks on the trajectory tab.

@achasmita
Copy link
Contributor Author

Here is the image for all the columns in the table.
Screen Shot 2023-09-19 at 8 02 59 PM

Screen Shot 2023-09-19 at 8 03 56 PM

Screen Shot 2023-09-19 at 8 04 40 PM

@shankari
Copy link
Contributor

@achasmita is this ready for review? It is still in draft mode...

Copy link
Contributor

@shankari shankari left a comment

Choose a reason for hiding this comment

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

This is a simple change so this is fine for now. But gentle reminder to add additional context to the issue as you explore increasingly complex implementations - e.g. when you read up on how to load the trajectories on demand, where did you find that from? How do you know that it works - e.g. how do you know that it doesn't load at startup?

Only requested change is a new code comment.

You also need to mark it as "non-draft" and resolve the conflict before I merge

pages/data.py Outdated
Comment on lines 79 to 81
elif tab == 'tab-trajectories-datatable':
if store_trajectories == {}:
store_trajectories = update_store_trajectories()
Copy link
Contributor

Choose a reason for hiding this comment

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

I am glad that you have this in a separate commit - it makes understanding the code easier. I think that this would also benefit from a short code comment on why it is different from the other tabs so that people have a rough sense without having to look at the blame immediately.

@achasmita achasmita marked this pull request as ready for review September 23, 2023 19:21
@shankari
Copy link
Contributor

I am merging this for now, but I was looking for more detailed comments, on the lines of:

when the user clicks any of the tabs, this function is invoked. The data for the other tabs is loaded at start.
but for this tab, we load the data if it has not yet been loaded.
however, if the data has already been loaded, we don't re-load
if the data changes while the dashboard is open..... (you need to fill this in).

@shankari shankari merged commit 37694ec into e-mission:master Sep 27, 2023
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