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.

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

FusionPower objective #1220

Merged
merged 15 commits into from
Aug 27, 2024
Merged

FusionPower objective #1220

merged 15 commits into from
Aug 27, 2024

Conversation

ddudt
Copy link
Collaborator

@ddudt ddudt commented Aug 22, 2024

Very similar to #1168, but this one is for output power.

@ddudt ddudt requested review from f0uriest and dpanici August 22, 2024 21:06
@ddudt ddudt self-assigned this Aug 22, 2024
@ddudt ddudt added the objectives Adding or improving objective functions label Aug 22, 2024
desc/compute/_equil.py Outdated Show resolved Hide resolved
desc/compute/_profiles.py Outdated Show resolved Hide resolved
desc/objectives/_power_balance.py Outdated Show resolved Hide resolved
Comment on lines 869 to 872
case "D(d,p)T":
energy = 1.01e6 + 3.02e6 # eV
case "D(d,n)3He":
energy = 0.82e6 + 2.45e6 # eV
Copy link
Member

Choose a reason for hiding this comment

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

The actual fuel in these two cases is both D+D. I think the "correct" way would be to have the user input the actual fuel, not just the reaction, and then we sum the energy/reactivity over the different possible reactions (which might be kind of annoying, especially if you want to account for further reactions between the products)

The other option would be to limit to just D+T and D+He3 since they only have 1 reaction path and generally no secondary reactions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK I switched this to fuel instead of reaction. For now let's only implement DT, and we can worry about adding more complicated fuels later if people want them.

One idea I though of is adding an additional kwarg to allow the user to specify the energy themselves if they want to include extra reactions between products (and if not supplied it would default to the basic case for that fuel). But I don't love adding lots of extra compute kwargs

@ddudt ddudt marked this pull request as ready for review August 23, 2024 17:55
@ddudt ddudt requested a review from f0uriest August 23, 2024 17:55
Base automatically changed from dd/iss04 to master August 25, 2024 22:41
@ddudt ddudt added the easy Short and simple to code or review label Aug 25, 2024
Copy link
Contributor

github-actions bot commented Aug 26, 2024

|             benchmark_name             |         dt(%)          |         dt(s)          |        t_new(s)        |        t_old(s)        | 
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
 test_build_transform_fft_lowres         |     -5.54 +/- 7.73     | -3.09e-02 +/- 4.31e-02 |  5.27e-01 +/- 4.0e-02  |  5.57e-01 +/- 1.7e-02  |
 test_build_transform_fft_midres         |     -4.39 +/- 7.73     | -2.81e-02 +/- 4.95e-02 |  6.12e-01 +/- 4.5e-02  |  6.40e-01 +/- 2.1e-02  |
 test_build_transform_fft_highres        |     -6.29 +/- 3.71     | -6.62e-02 +/- 3.90e-02 |  9.86e-01 +/- 3.7e-02  |  1.05e+00 +/- 1.2e-02  |
 test_equilibrium_init_lowres            |    -10.13 +/- 3.69     | -4.26e-01 +/- 1.55e-01 |  3.77e+00 +/- 7.6e-02  |  4.20e+00 +/- 1.3e-01  |
 test_equilibrium_init_medres            |     -1.83 +/- 11.40    | -7.66e-02 +/- 4.78e-01 |  4.11e+00 +/- 1.5e-01  |  4.19e+00 +/- 4.5e-01  |
 test_equilibrium_init_highres           |     -2.17 +/- 11.72    | -1.21e-01 +/- 6.55e-01 |  5.46e+00 +/- 1.8e-01  |  5.59e+00 +/- 6.3e-01  |
+test_objective_compile_dshape_current   |     -8.06 +/- 1.78     | -3.34e-01 +/- 7.38e-02 |  3.81e+00 +/- 6.8e-02  |  4.14e+00 +/- 2.8e-02  |
+test_objective_compile_atf              |     -7.55 +/- 1.02     | -6.46e-01 +/- 8.69e-02 |  7.91e+00 +/- 6.3e-02  |  8.55e+00 +/- 6.0e-02  |
 test_objective_compute_dshape_current   |     -2.51 +/- 1.99     | -9.03e-05 +/- 7.14e-05 |  3.51e-03 +/- 5.2e-05  |  3.60e-03 +/- 4.9e-05  |
+test_objective_compute_atf              |    -14.09 +/- 2.60     | -1.68e-03 +/- 3.11e-04 |  1.03e-02 +/- 9.7e-05  |  1.20e-02 +/- 3.0e-04  |
 test_objective_jac_dshape_current       |     -4.36 +/- 7.23     | -1.86e-03 +/- 3.08e-03 |  4.07e-02 +/- 2.4e-03  |  4.26e-02 +/- 2.0e-03  |
 test_objective_jac_atf                  |     -4.99 +/- 3.06     | -1.01e-01 +/- 6.21e-02 |  1.93e+00 +/- 4.2e-02  |  2.03e+00 +/- 4.6e-02  |
 test_perturb_1                          |     -8.47 +/- 10.36    | -1.16e+00 +/- 1.42e+00 |  1.25e+01 +/- 4.4e-01  |  1.37e+01 +/- 1.4e+00  |
 test_perturb_2                          |     +8.24 +/- 6.76     | +1.45e+00 +/- 1.19e+00 |  1.91e+01 +/- 4.8e-01  |  1.76e+01 +/- 1.1e+00  |
 test_proximal_jac_atf                   |     +1.73 +/- 2.03     | +1.42e-01 +/- 1.67e-01 |  8.34e+00 +/- 1.3e-01  |  8.20e+00 +/- 1.0e-01  |
 test_proximal_freeb_compute             |     -0.35 +/- 1.85     | -6.50e-04 +/- 3.40e-03 |  1.83e-01 +/- 2.8e-03  |  1.84e-01 +/- 1.9e-03  |
 test_proximal_freeb_jac                 |     -1.95 +/- 1.07     | -1.46e-01 +/- 8.03e-02 |  7.33e+00 +/- 5.6e-02  |  7.47e+00 +/- 5.8e-02  |
 test_solve_fixed_iter                   |     +1.41 +/- 60.22    | +7.07e-02 +/- 3.01e+00 |  5.07e+00 +/- 2.1e+00  |  5.00e+00 +/- 2.2e+00  |

Copy link

codecov bot commented Aug 26, 2024

Codecov Report

Attention: Patch coverage is 93.75000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 95.32%. Comparing base (fd73bdb) to head (fd6b57c).
Report is 2041 commits behind head on master.

Files with missing lines Patch % Lines
desc/objectives/_power_balance.py 90.69% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1220      +/-   ##
==========================================
- Coverage   95.33%   95.32%   -0.01%     
==========================================
  Files          90       90              
  Lines       22581    22643      +62     
==========================================
+ Hits        21527    21585      +58     
- Misses       1054     1058       +4     
Files with missing lines Coverage Δ
desc/compute/_equil.py 100.00% <100.00%> (ø)
desc/compute/_profiles.py 99.19% <100.00%> (+0.02%) ⬆️
desc/objectives/__init__.py 100.00% <100.00%> (ø)
desc/objectives/_power_balance.py 89.58% <90.69%> (ø)

... and 2 files with indirect coverage changes

dpanici
dpanici previously approved these changes Aug 26, 2024
fuel not in ["DT"], ValueError, f"fuel must be one of ['DT'], got {fuel}."
)
if target is None and bounds is None:
target = 0
Copy link
Member

Choose a reason for hiding this comment

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

Is this the most sensible default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A good target will depend on the scale of the machine, but I changed it to 1,000 MW to be realistic for a power plant

@ddudt ddudt requested a review from f0uriest August 27, 2024 00:02
f0uriest
f0uriest previously approved these changes Aug 27, 2024
dpanici
dpanici previously approved these changes Aug 27, 2024
Copy link
Collaborator

@dpanici dpanici left a comment

Choose a reason for hiding this comment

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

Approving, but is Fusion power density something that could be of interest/be optimized? would it make sense to have one compute quantity be local fusion power density and then another that does the integration to get total power?

@daniel-dudt daniel-dudt dismissed stale reviews from dpanici and f0uriest via fd6b57c August 27, 2024 14:56
@ddudt
Copy link
Collaborator Author

ddudt commented Aug 27, 2024

Approving, but is Fusion power density something that could be of interest/be optimized? would it make sense to have one compute quantity be local fusion power density and then another that does the integration to get total power?

If someone wants that they can add it in another PR.

@ddudt ddudt requested review from f0uriest and dpanici August 27, 2024 14:58
@ddudt ddudt merged commit 225d44e into master Aug 27, 2024
17 of 18 checks passed
@ddudt ddudt deleted the dd/bh-power branch August 27, 2024 18:30
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 objectives Adding or improving objective functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants