Skip to content

Comments

Improve efficiency - refactoring#251

Merged
aclark02-arcus merged 39 commits intodevfrom
ls_improve_efficiency
Dec 29, 2025
Merged

Improve efficiency - refactoring#251
aclark02-arcus merged 39 commits intodevfrom
ls_improve_efficiency

Conversation

@LDSamson
Copy link
Collaborator

@LDSamson LDSamson commented Dec 2, 2025

Partly addressess #223. With this PR, no app_tables object needs to be created when starting the app anymore. Since app_tables were created twice (at start up and once when loading each form for the first time), this PR improves efficiency.

It would also mean one less object to precalculate when implementing #245, reducing complexity.
@aclark02-arcus do you have time to take a look here? If not please let me know, thanks!

@LDSamson LDSamson marked this pull request as ready for review December 2, 2025 15:36
@aclark02-arcus
Copy link
Collaborator

aclark02-arcus commented Dec 4, 2025

Unfortunately, I don't have a lot of time to review all of this right now, as it's a pretty big / pervasive change, but I see how it would impact #245.

From what perspective does this improve efficiency? Does it just reduce the amount of R objects floating around, being passed to functions? Or does it impact app run time processes?

@LDSamson
Copy link
Collaborator Author

LDSamson commented Dec 5, 2025

Unfortunately, I don't have a lot of time to review all of this right now, as it's a pretty big / pervasive change, but I see how it would impact #245.

From what perspective does this improve efficiency? Does it just reduce the amount of R objects floating around, being passed to functions? Or does it impact app run time processes?

A few things:

  • Previously, app_tables needed to be created, just to retrieve some data for a few other helper tables. In addition to that, each table is also created when viewing a form in mod_common_forms/mod_study_forms etc. Now app_tables is not needed anymore. For big data sets I think this saves a lot of unneeded calculations/memory allocation
  • the number of visits was retrieved three times in three different tables from the appdata object in a not so efficitent way. This has been reduced to one only time.

It all just works more together now, removing several duplications.

Note that we could also convert the timeline_data to a normal data frame so that it can be created in advance as well (see #245).

@aclark02-arcus
Copy link
Collaborator

Note that we could also convert the timeline_data to a normal data frame so that it can be created in advance as well (see #245).

That would be amazing!

@LDSamson
Copy link
Collaborator Author

LDSamson commented Dec 5, 2025

Note that we could also convert the timeline_data to a normal data frame so that it can be created in advance as well (see #245).

That would be amazing!

Adjusted, done :)

Copy link
Collaborator

@aclark02-arcus aclark02-arcus left a comment

Choose a reason for hiding this comment

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

Hi @LDSamson, this looks good to me. I didn't test it on our internal data, but I played around with it for a while and couldn't find any issues. I'll merge and if there are any issues, we can address them in a follow-up PR.

@aclark02-arcus aclark02-arcus merged commit 1152106 into dev Dec 29, 2025
5 checks passed
@LDSamson LDSamson deleted the ls_improve_efficiency branch January 5, 2026 06:59
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