Skip to content

update comments#2242

Open
unalmis wants to merge 6 commits into
masterfrom
unalmis-patch-1
Open

update comments#2242
unalmis wants to merge 6 commits into
masterfrom
unalmis-patch-1

Conversation

@unalmis

@unalmis unalmis commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

Adds some comments to objectives

Added notes about a more performant version of the implementation.
@unalmis unalmis added skip_changelog No need to update changelog on this PR only-docs-comments Don't run workflows if the changes are only on the comments labels Jun 15, 2026
@unalmis unalmis marked this pull request as ready for review June 15, 2026 22:30
@unalmis unalmis mentioned this pull request Jun 15, 2026
@unalmis unalmis requested review from a team, YigitElma, ddudt, dpanici, f0uriest and rahulgaur104 and removed request for a team June 15, 2026 22:31
@unalmis unalmis added the easy Short and simple to code or review label Jun 15, 2026
@unalmis unalmis added the P∞ P_infty. Ready to merge > 1 years. priority to merge to prevent further delay of research. label Jun 17, 2026

@rahulgaur104 rahulgaur104 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Full optimization performance benchmarks were never performed. For example, when optimizing an equilibrium, the new bounce integral from ku/bounce (merged) took almost exactly the same time as the old bounce integration routines. So compute maybe faster but optimization definitely was not. Maybe sparse pullback will change that, maybe not.

I don't actually care at this point, so approving this PR.

@unalmis

unalmis commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator Author

Full optimization performance benchmarks were never performed. For example, when optimizing an equilibrium, the new bounce integral from ku/bounce (merged) took almost exactly the same time as the old bounce integration routines. So compute maybe faster but optimization definitely was not. Maybe sparse pullback will change that, maybe not.
I don't actually care at this point, so approving this PR.

@rahulgaur104 's comment purpose is not clear to me, nor why he feels the need to advertise he does not care.

Focusing on the technical/work relevant content of that comment, his statements in reference to my work are verifiably false. To reduce spread of misinformation for others who are reading, note that

  • ku/bounce (link: New inverse stream map to accelerate convergence #1919 ) is a branch that primarily focused on correcting bugs in DESC and improving convergence; not performance.
  • Still, the performance benefit from that PR is significant if one benchmarks it properly - e.g. compare how much faster it is when a low resolution solver is used (because the convergence improvements make the new low resolution solver as accurate as the old high resolution solver).
  • All of this is shown in detail in New inverse stream map to accelerate convergence #1919 .

Next, ku/sparse_pullback (link: #2170 ) and the following PRs reduces memory and improves performance by a factor of the number of flux surfaces the optimization is done with.

I suggest making effort to become better informed before making negative claims on others' work.

@unalmis unalmis added run_benchmarks Run timing benchmarks on this PR against current master branch override codecov Override codecov labels Jun 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

easy Short and simple to code or review only-docs-comments Don't run workflows if the changes are only on the comments override codecov Override codecov P∞ P_infty. Ready to merge > 1 years. priority to merge to prevent further delay of research. run_benchmarks Run timing benchmarks on this PR against current master branch skip_changelog No need to update changelog on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants