Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Analyser/Generator: cache variable equivalences #763

Merged
merged 34 commits into from
Dec 6, 2020
Merged

Analyser/Generator: cache variable equivalences #763

merged 34 commits into from
Dec 6, 2020

Conversation

agarny
Copy link
Contributor

@agarny agarny commented Oct 26, 2020

Addresses issue #762.

@agarny agarny linked an issue Oct 26, 2020 that may be closed by this pull request
Analyser/Generator: cache variable equivalences #762
Closed
@agarny
Copy link
Contributor Author

agarny commented Oct 26, 2020

Some quick tests show that, on my Mac, Generator.fabbriFantiniWildersSeveriHumanSanModel2017 is now ~33% faster to run while Generator.garnyKohlHunterBoyettNobleRabbitSanModel2003 is ~27.5% faster. Better than nothing, I guess, and no doubt that there is room for improvement in other places.

@agarny agarny requested review from kerimoyle, hsorby and nickerso and removed request for kerimoyle October 26, 2020 07:04
nickerso
nickerso reviewed Oct 26, 2020
View reviewed changes
cmake/common.cmake Outdated Show resolved Hide resolved
@agarny
Some minor cleaning up.
7c5bc6a
@agarny
AnalyserModel: renamed isSameOrEquivalentVariable() to `areEquivale…
1bdb653 
…ntVariables()`.
@agarny
AnalyserModel: added some background information to the `areEquivalen…
dd542cf 
…tVariables()` method.
@agarny
Generator: re-made some methods constants.
6e55d5b 
`const` had been removed from them while working on a different implementation, but now it's fine: we can put `const` back.
@agarny agarny requested a review from nickerso October 27, 2020 20:41
@agarny
Copy link
Contributor Author

agarny commented Oct 27, 2020

Ready to be (re-)reviewed, @hsorby, @kerimoyle and @nickerso.

hsorby
hsorby previously requested changes Nov 1, 2020
View reviewed changes
Copy link
Contributor

@hsorby hsorby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor change for me otherwise happy with this.

src/generator.cpp Outdated Show resolved Hide resolved
@agarny agarny requested a review from hsorby November 1, 2020 23:29
@agarny
Copy link
Contributor Author

agarny commented Nov 1, 2020

Ready to be (re-)reviewed, @hsorby, @kerimoyle and @nickerso.

nickerso
nickerso requested changes Nov 2, 2020
View reviewed changes
src/analysermodel.cpp Outdated Show resolved Hide resolved
src/analysermodel.cpp Outdated Show resolved Hide resolved
src/api/libcellml/analysermodel.h Outdated Show resolved Hide resolved
@agarny agarny dismissed hsorby’s stale review December 1, 2020 22:01

All the issues have been addressed.

@agarny
Copy link
Contributor Author

agarny commented Dec 1, 2020

Ping @hsorby, @kerimoyle and @nickerso: good to go.

hsorby
hsorby reviewed Dec 2, 2020
View reviewed changes
Copy link
Contributor

@hsorby hsorby left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a problem with the assumption of a fixed size because this will not work with emscripten. I would prefer for this to be fixed now but I am okay with leaving it until emscripten properly joins the codebase.

Otherwise I am happy with the changes.

src/analysermodel.cpp Outdated Show resolved Hide resolved
@agarny agarny mentioned this pull request Dec 2, 2020
Closed
@agarny
Analyser model: cast shared_ptr::get() to a portable type.
e7a40a5 
So, that we can create JavaScript bindings using Emscripten.
hsorby
hsorby previously approved these changes Dec 4, 2020
View reviewed changes
Copy link
Contributor

@hsorby hsorby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is good to go.

@agarny agarny requested a review from hsorby December 6, 2020 20:23
hsorby
hsorby requested changes Dec 6, 2020
View reviewed changes
src/analysermodel_p.h Outdated Show resolved Hide resolved
nickerso
nickerso previously approved these changes Dec 6, 2020
View reviewed changes
@hsorby hsorby merged commit d229620 into cellml:develop Dec 6, 2020
@agarny agarny deleted the cache-isSameOrEquivalentVariable branch December 6, 2020 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hsorby hsorby hsorby approved these changes

@nickerso nickerso nickerso approved these changes

@kerimoyle kerimoyle Awaiting requested review from kerimoyle

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Analyser/Generator: cache variable equivalences
4 participants
@agarny @hsorby @nickerso @kerimoyle