Up to 7x faster _save_outputs!()

[ll-ara]

Changed 'for' loops to 'map' to remove a bottleneck.

The code is very similar but note that I have changed a dictionary to a vector of pairs with collect(). [Edit: not anymore, it's a dictionary again :)]

Checklist before merging

• Documentation is up-to-date
• No extra unit tests needed?
• Code has been formatted according to SpineOpt's style
• Unit tests pass

[DillonJ]

Excellent work @ll-ara - this is great! Do you need help with the tests or do you plan on working on those?

[ll-ara]

Thanks @DillonJ, I'm currently working on getting it to pass the existing unit test cases - I'll let you know if I need help.
I'm assuming we don't need to create new unit test cases because it should be well covered by the existing ones already?

[ll-ara]

I have fixed my silly copy-paste mistake + changed it back to a Dict and the unit tests are passing now on my machine.

What’s the next step? Running the CI?

[DillonJ]

Great - CI usually runs automatically... maybe we are out of resources? @datejada do you know why we are not seeing the tests running automatically?

[tarskul]

I'm not sure whether this is the case, but we may be out of budget. Every month there is a certain amount of hours that we can run such tests. Currently we seem to have already used double the resources that we used last month. Though I would imagine we would get a notification of this. But I'm not ruling it out either.

[DillonJ]

@ll-ara I suppose we can wait till the start of the month - but I'd be eager to get this one merged asap. I'm also hoping you find more 7x improvements :-) Please see issues #1216 #1218 and #1219. It would be great if you could contribute!!

[datejada]

@DillonJ and @tarskul The CI workflow was not triggered for this pull request because there are no workflow runs associated with the pull request because there is no GitHub Actions workflow configured to run on pull_request events in SpineOpt. The workflows are configured to only run for pull requests from branches within the main repository, and this PR comes from a fork (ll-ara/SpineOpt.jl).

We may want to modify the workflow to run on pull_request event as well.

Two options here:

1. We modify the workflow to allow forks to run the CI
2. We push this PR to main to run the CI

I prefer 1 since PR on forks can be merged without any checks. So, we still need to update the CI to avoid this.

LEt me know what you think and then we can proceed

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.68%. Comparing base (c5b821a) to head (adf34ba).
Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1217   +/-   ##
=======================================
  Coverage   87.68%   87.68%           
=======================================
  Files         147      147           
  Lines        4532     4532           
=======================================
  Hits         3974     3974           
  Misses        558      558           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[datejada]

@ll-ara, I updated our CI to run the tests on the PR. It looks good. Thanks! One more thing, could you share your tests of logs where you get the x7 speed up?

[ll-ara]

@datejada sorry not quite sure what you mean? I added time() calls to the code and printed it out. Do you mean what inputs I used?

[datejada]

@datejada sorry not quite sure what you mean? I added time() calls to the code and printed it out. Do you mean what inputs I used?

Sorry, I didn't explain myself correctly. I would like to know in which case/example you got the 7x faster. Was it in one of the examples we have or in your own case study. If it is the later, it will be nice to know the size of the case study you got the improvement.

Thanks!

[ll-ara]

The size was small: only around 15 objects in the rolling optimization