Skip to content

Add option to select activation function for ChamberSphere - #239

Open
charoheijnen wants to merge 7 commits into
SimVascular:masterfrom
charoheijnen:newchamber
Open

Add option to select activation function for ChamberSphere#239
charoheijnen wants to merge 7 commits into
SimVascular:masterfrom
charoheijnen:newchamber

Conversation

@charoheijnen

Copy link
Copy Markdown

Add option to select activation function for ChamberSphere

Current situation

Currently activation function is hard coded within ChamberSphere. As detailed in #236

Release Notes

  • added option to choose activation funciton to chambersphere block
  • added activaion funtion from chambersphere to activation funciton block

Testing

  • chambersphere test case still runs as previously when choosing double tanh activation funciton with parameters that were in previous test case.

Code of Conduct & Contributing Guidelines

…y of activation funcitons. Added double_tanh activation funciton to activation function file. Changed chamver_sphere.json test case to be able to choose activaiton function. when double_tanh activation funciton was selected test case runs as previously ran.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@KatrinKoesler
KatrinKoesler requested a review from mrp089 July 22, 2026 15:38
@mrp089

mrp089 commented Jul 24, 2026

Copy link
Copy Markdown
Member

@charoheijnen, it looks like the GUI test is throwing an error:

Node 20 is being deprecated. This workflow is running with Node 24 by default.

I think the fix is to bump this up to 24 (or avoid setting a version altogether):

node-version: '20'

@mrp089 mrp089 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great work, @charoheijnen! See below two comments

Comment thread src/solve/SimulationParameters.cpp
Comment thread src/solve/SimulationParameters.cpp

@mrp089 mrp089 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think SimulationParameters can be simplified. See comments.

model, vessel_config["activation_function"], vessel_name);
model.get_block(vessel_name)
->set_activation_function(std::move(act_func));
}

@mrp089 mrp089 Aug 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is this extra check necessary? There is already the function create_chambers that creates an activation function for all chambers besides ChamberSphere. Can you simply add ChamberSphere to that list? Even better: If now all chambers use an activation function, the lengthy if-statement in create_chambers could be removed entirely.

model.cardiac_cycle_period = period;
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why does this need to be moved? Maybe with the change to create_chambers that can stay where it is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants