Improve caching of equivalent variables in analyser model.#1389
Improve caching of equivalent variables in analyser model.#1389hsorby wants to merge 13 commits into
Conversation
agarny
left a comment
agarny
left a comment
There was a problem hiding this comment.
Looks good and definitely faster! Nice!
AnalyserModel::AnalyserModelImpl::buildEquivalentVariablesCache() should probably be called in AnalyserModel::areEquivalentVariables() (if the cache hasn't already been built), but having said that we know that the analyser is going to call AnalyserModel::areEquivalentVariables() many times, so I guess it's fine to call AnalyserModel::AnalyserModelImpl::buildEquivalentVariablesCache() upon retrieving the analyser model?
agarny
left a comment
agarny
left a comment
There was a problem hiding this comment.
This looks. "Just" some nitpicking.
| * | ||
| * @note This function is primarily designed for use during model analysis | ||
| * by the @ref Analyser. While external usage is not programmatically | ||
| * restricted, it is not the primary intended use case. |
There was a problem hiding this comment.
is there a reason to "restrict" this? variable equivalence is a common aspect of CellML and many uses outside of libCellML could take advantage of a convenient and fast equivalence cache if it was pulled out of the analyser and made available as a first class part of the libCellML API. Similar to the CellML API's CeVAS service to manage connected variables...
There was a problem hiding this comment.
It isn’t “restricted”, the note is trying to explain that the function may find uses by the user that the developers have not considered, and that its primary function is to facilitate fast equivalent variable lookups for the analyser. The user needs to understand the limitations of this function if they do use if for their own purposes.
If this note doesn’t convey that message in a shorter form then maybe it needs re-writing.
There was a problem hiding this comment.
restricted in that its part of the analyser. I'm wondering how hard it would be to pull the equivalence cache out into its own class or maybe part of a model or something... Analysing models can only be done on complete models, whereas variable equivalence can be done any time.
There was a problem hiding this comment.
I don’t think that would be hard at all. Either option model based cache or first class object EquivalentVariableCache would work.
My initial thought would be to have it as a first class object to keep the model as a plain model (arguments here could be made for some of the action API that currently exist on the model). I think I would prefer to see the model become plainer over time and have actioners do work on the model. The equivalent variable cache could be one of those. I expect this would become clearer once some exploratory tests were written.
I do see this query as being a separate issue to what this pull request is addressing.
3 participants
Uh oh!
There was an error while loading. Please reload this page.