Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[tools] Fix efficiency of fix_candidate_age script #8992

Merged

Conversation

CamilleBeau
Copy link
Contributor

Brief summary of changes

This PR fixes a bug in the fix_candidate_age.php script where it is getting CommentIDs from every instrument for each instrument, instead of just getting commentIDs for that instrument. This is making the script take n times as long where n = the number of instruments on the project.

Testing instructions (if applicable)

  1. Run the script
  2. Make sure that it runs successfully and in a reasonable time scale.

@driusan
Copy link
Collaborator

driusan commented Dec 1, 2023

If the problem you're trying to solve is efficiency can you also update it to use bulkLoadInstanceData rather than calling factory n times? I think that would also be a (potentially much bigger) improvement in the efficiency.

@CamilleBeau
Copy link
Contributor Author

If the problem you're trying to solve is efficiency can you also update it to use bulkLoadInstanceData rather than calling factory n times? I think that would also be a (potentially much bigger) improvement in the efficiency.

Thanks Dave, done!

@driusan driusan merged commit 47637c8 into aces:24.1-release Dec 1, 2023
9 checks passed
driusan pushed a commit that referenced this pull request Jan 23, 2024
This resolves the data shift issue by adding the number of valid samples in the last chunk to the index.json, and using it for the calculations which previously assumed all chunks were filled.

Closes #8992. Has a Loris-MRI counterpart: aces/Loris-MRI#1030

Loosely dependant on #8998 (file modifications in common and done at a later time).
jeffersoncasimir added a commit to jeffersoncasimir/Loris that referenced this pull request Feb 29, 2024
This resolves the data shift issue by adding the number of valid samples in the last chunk to the index.json, and using it for the calculations which previously assumed all chunks were filled.

Closes aces#8992. Has a Loris-MRI counterpart: aces/Loris-MRI#1030

Loosely dependant on aces#8998 (file modifications in common and done at a later time).
@ridz1208 ridz1208 added this to the 24.1.6 milestone Apr 9, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants