Use std::unique_ptrs for storing model definitions

[alexdewar]

I fancied tinkering with some code rather than just config files, so I did this, even though it's not in the backlog and is a pretty minor refactor 😛.

Currently, the model definitions are stored in the repository (of type CachedRepository) as std::shared_ptrs, even though they don't really need to be "shared", because the repository object will outlive any of the users of these model definitions. So instead, I've changed these to be std::unique_ptrs; when they're needed they can be accessed as good old-fashioned const refs.

I also changed a couple of small issues I spotted on the way.

[dalonsoa]

James explained to me the difference between shared_ptr and unique_ptr and while I did not grasped everything, I gathered that using the second option was not possible in this case since we need to have multiple copies of it in different places. Needless to say that I'm not an expert in C++, so maybe I got this totally wrong....

[alexdewar]

Yeah, I made this suggestion on @jamesturner246's PR and we did come to the conclusion that it probably needed to be a std::shared_ptr. I took another look though and the thing is that it seems more sensible to just have the repository own the model definitions (i.e. destroy them along with itself), because the repository outlives any of the users of them. They can be accessed as references during this time.

std::shared_ptr is what you need where your object effectively has multiple "owners", e.g. you've copied your shared_ptr between threads. In that case, you don't know which part of your code (thread) will need the object last, so a shared_ptr will only delete it when it's the last shared_ptr for the object around. That would work here too, but it's simpler just to have one owner and have everyone else access it as a reference.

[jamesturner246]

There's a chance that they want to be able to modify model parameters -- i.e. contents of model definition -- mid simulation. I'm not yet decided on where it might be modified from, but would this be a faff to change to non-const in future, should we need to modify from EnergyBalanceModel?

tbh I don't like these 'definition' classes anyway, and getting rid of them would mean we would only modify the model object directly.

[alexdewar]

That sounds v bizarre 😆 but I'm sure there's a good reason for it! Yes, you could easily modify it to be non-const. The thing is though, if you do that it will also modify the version in the repository (and therefore any future models will also have modified params). Would that be a problem? If so, it would probably be best just to copy the model definitions into the model classes, so they have their own copies to muck about with.

I did wonder if we might be able to remove the model definition classes, but having had a look just now, I'm wondering how we'd be able to construct models without having the params encapsulated in a separate class somewhere? (That's assuming that we actually do want to cache model parameters rather than reloading them every time.)

[jamesturner246]

The only sensible reason I could think of for caching parameters was if they wanted some persistent interface in future, where they could stop, reinitialise, restart, etcetera, which would mean re-initialising from some pristine confifg. In which case, you're right, it's probably better to keep the 'definition' classes, keep the cache constant, and just copy out of it whenever they need fresh state.

If they don't want that, then there's no reason to cache anything. But I suppose they might want that in future, so I guess that's reason enough to keep the pristine cache.

I imagine a reset_model method of the 'definition' classes, which takes a model instance the same type its create_model method generates. Okay, cool, happy to merge this.

[dalonsoa]

Rather than wondering what they might want, why do you ask them (after giving some context of the issue)?

[jamesturner246]

I'll bring it up with them this week.