[REFACTOR] Avoid computing samples eagerly in SimulationResults

[mattdailis]

• Tickets addressed: N/A
• Review: By commit
• Merge strategy: Merge (no squash)

Description

This PR removes resourceSamples as a field on SimulationResults. Prior to this PR, this field was derived from realProfiles and discreteProfiles via the takeSamples method - upon calling the SimulationResults constructor, takeSamples would be called eagerly.

In typical usage, resourceSamples is unused - the simulation results are written to the database using the realProfiles and discreteProfiles directly. The only time it is used (outside of testing) is via the explicit resourceSamples hasura action.

This PR moves computation of resource samples out of the SimulationResults constructor, so that they can be computed when requested, but not by default.

I have not done any performance measurements to see whether or not this change is significant - nevertheless, I believe avoiding needless computations is generally a good thing.

Verification

I spun up a local Aerie deployment and checked that running simulations and viewing their results worked, and I also called the resourceSamples endpoint directly.

Documentation

No functional behavior has changed, so no documentation needs to be updated.

Future work

No future work

[Mythicaeda]

Looks fine, although seeing it appear in the mission models makes me wonder how often this calculation will be called per SimulationResults object. Would it be worth it to instead make resourceSamples a private field that is not assigned at construction, and have it accessed through a getter that assigns the output of takeSamples into it if resourceSamples is null? Then we only run takeSamples at most once per SimulationResults object.

[mattdailis]

Looks fine, although seeing it appear in the mission models makes me wonder how often this calculation will be called per SimulationResults object. Would it be worth it to instead make resourceSamples a private field that is not assigned at construction, and have it accessed through a getter that assigns the output of takeSamples into it if resourceSamples is null? Then we only run takeSamples at most once per SimulationResults object.

If we expected this computation to happen multiple times, absolutely, I think caching it in a field would be a valid approach. 💯

As it stands, we've moved away from this method (it provides strictly less information than realProfiles and discreteProfiles do, while using more memory) - so we don't expect this method to get more use in the future. If I had my druthers, we'd remove this method entirely, but we're keeping the resourceSamples hasura action in the API as a convenience for clients that don't want to work with continuous profiles.

[Mythicaeda]

As it stands, we've moved away from this method (it provides strictly less information than realProfiles and discreteProfiles do, while using more memory) - so we don't expect this method to get more use in the future. If I had my druthers, we'd remove this method entirely, but we're keeping the resourceSamples hasura action in the API as a convenience for clients that don't want to work with continuous profiles.

So this refactor is setup for an eventual deprecation? If so, then I think I'm fine with making it the responsibility of whoever calls the public method to cache its return value.

[mattdailis]

So this refactor is setup for an eventual deprecation? If so, then I think I'm fine with making it the responsibility of whoever calls the public method to cache its return value.

When I brought up the possibility of deprecation, @patkenneally explained that we should keep the capability in case API clients want it, but that we should encourage people to use the continuous profiles directly. So - yes, it's "discouraged" if not exactly "deprecated for removal".

[Twisol]

Personally, I'd like to see the resourceSamples code moved out of SimulationResults entirely. It looks like the only place where we access it outside of testing/development code is that API endpoint, and the computation doesn't really feel like the responsibility of SimulationResults anymore.

[mattdailis]

Personally, I'd like to see the resourceSamples code moved out of SimulationResults entirely

Done 👍 The sampling logic is now the responsibility of the GetSimulationResultsAction, which was the only client of this functionality. I inlined it into the getResourceSamples method.

[Twisol]

Awesome! Thanks for doing that!