Skip to content

bug: TestMakeALookupTable does not accept precompiled model names#32

Merged
mirams merged 2 commits into
Chaste:mainfrom
martinjrobins:master
Apr 23, 2025
Merged

bug: TestMakeALookupTable does not accept precompiled model names#32
mirams merged 2 commits into
Chaste:mainfrom
martinjrobins:master

Conversation

@martinjrobins
Copy link
Copy Markdown
Member

No description provided.

@mirams mirams self-assigned this Mar 24, 2025
@mirams mirams added the bug label Mar 24, 2025
@mirams mirams self-requested a review March 24, 2025 12:16
@mirams mirams removed their assignment Mar 24, 2025
@mirams
Copy link
Copy Markdown
Member

mirams commented Mar 27, 2025

I had a quick play with this @martinjrobins and it seems to work for a precompiled CellML that is not in the original 1-9 indices, but not for a runtime compiled one. Would it be easy enough to tweak for that too?

@martinjrobins
Copy link
Copy Markdown
Member Author

Would it be easy enough to tweak for that too?

Sure, I didn't think of the runtime ones, I'll give that a go

@mirams
Copy link
Copy Markdown
Member

mirams commented Mar 27, 2025

I guess there could be an argument for banning that, as it probably is a good idea to have some kind of stability in the model/executable such that someone hasn't changed the cellml file on a regular basis and gets stale lookup tables... but on the other hand it would make it more consistent with the SetupModel to be able to do it

@martinjrobins
Copy link
Copy Markdown
Member Author

I had a look at this and think it will be tricky. You are creating one SetupModel in each thread, and each SetupModel is responsible for compiling the cellml file to get the model, so multiple threads will be trying to copy into the same directory and compile the cellml file. I think you'd have to restructure the code quite a bit to to this, not sure if its worth it if there is an argument for banning it....

@mirams mirams merged commit 6dd9a1c into Chaste:main Apr 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants