Skip to content

322 solution should have explicit voltage attribute #325

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
Merged
merged 4 commits into from
May 30, 2025
Merged

322 solution should have explicit voltage attribute #325

merged 4 commits into from
May 30, 2025

Conversation

@peterhollender
Copy link
Contributor

@peterhollender peterhollender commented May 29, 2025

Closes #322

@peterhollender peterhollender linked an issue May 29, 2025 that may be closed by this pull request
Solution should have explicit "Voltage" attribute #322
Closed
peterhollender added a commit that referenced this pull request May 30, 2025
@peterhollender
update dvc to merge #325 changes with main changes
782ced5
ebrahimebrahim
ebrahimebrahim approved these changes May 30, 2025
View reviewed changes
Copy link
Collaborator

@ebrahimebrahim ebrahimebrahim left a comment

Choose a reason for hiding this comment

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

All looks good, and I tested ended to end in SlicerOpenLIFU that the voltage ends up in the right place and so on.

Thank you for "merging" the DVC data after the last update :)
(DVC doesn't provide great merging tools unfortunately)

@ebrahimebrahim
Copy link
Collaborator

ebrahimebrahim commented May 30, 2025 •
edited
Loading

Heads up that I am going to manually rebase and force push! It is needed to handle some conflicts

@peterhollender
Copy link
Contributor Author

peterhollender commented May 30, 2025

All looks good, and I tested ended to end in SlicerOpenLIFU that the voltage ends up in the right place and so on.

Thank you for "merging" the DVC data after the last update :) (DVC doesn't provide great merging tools unfortunately)

Great. @alkagan tested it with the hardware and confirmed that it's all good with the test scripts.

@peterhollender @ebrahimebrahim
add voltage to solution
b479d39 
Closes #322
@peterhollender @ebrahimebrahim
Update pulse.py
0dcff9d 
Adjusts pulse amplitude to be arbitrary units per #322
@peterhollender @ebrahimebrahim
Update LIFUInterface
bc8b932 
Changes LIFUInterface.set_solution to pull the new voltage attribute per #322
@ebrahimebrahim ebrahimebrahim force-pushed the 322-solution-should-have-explicit-voltage-attribute branch from 782ced5 to b905811 Compare May 30, 2025 19:22
ebrahimebrahim pushed a commit that referenced this pull request May 30, 2025
@peterhollender @ebrahimebrahim
update dvc to merge #325 changes with main changes
b905811
@ebrahimebrahim ebrahimebrahim force-pushed the 322-solution-should-have-explicit-voltage-attribute branch from b905811 to 1da4b93 Compare May 30, 2025 19:24
@ebrahimebrahim
Copy link
Collaborator

ebrahimebrahim commented May 30, 2025

(Not important but in case you are wondering why I reworded the "update DVC" commit to just say it is updating per the present issue: even though you did do a "merge", the DVC history always appears linear, so this particular DVC update appears to make only the changes for #322)

@ebrahimebrahim ebrahimebrahim enabled auto-merge (rebase) May 30, 2025 19:27
@ebrahimebrahim ebrahimebrahim merged commit 5516547 into main May 30, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ebrahimebrahim ebrahimebrahim ebrahimebrahim approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Solution should have explicit "Voltage" attribute

3 participants

@peterhollender @ebrahimebrahim