Mirror model in ACCERT - #59
Conversation
|
Hi @NybergWISC could you please review this PR? In particular, please check:
|
NybergWISC
left a comment
NybergWISC
left a comment
There was a problem hiding this comment.
Only a couple formatting comments based on Mirror being a part of general fusion models. It may be worth (soon) transitioning to naming the "fusion" model the "tokamak" model, but that doesn't need to be a part of this PR. From what I can see, the baseline values are in-line with the values from the runs used for the draft of the paper and models from Frank et al and Shah et al.
| ====================== | ||
|
|
||
| This document provides a comprehensive explanation of the ACCERT running examples, including the **PWR12-BE**, **ABR1000**, **AP1000**, and **Fusion** reactor models. It integrates key ACCERT concepts to elucidate how the input files are structured and why they are designed in a particular manner. | ||
| This document provides a comprehensive explanation of the ACCERT running examples, including the **PWR12-BE**, **ABR1000**, **AP1000**, **Fusion**, and **Mirror** reactor models. It integrates key ACCERT concepts to elucidate how the input files are structured and why they are designed in a particular manner. |
There was a problem hiding this comment.
May be worth changing the name of the "Fusion" reactor models to a list of the specific types or removing the extra mirror that is added here (dropping this change)
| @@ -16,3 +16,4 @@ This document provides a comprehensive explanation of the ACCERT running example | |||
| ap1000 | |||
| fusion | |||
There was a problem hiding this comment.
Again, it may be worth changing "fusion" to "tokamak" as we start adding in more capabilities
| } | ||
| for table_name, csv_path in expected.items(): | ||
| cursor.execute(f"SELECT COUNT(*) FROM {table_name}") | ||
| assert cursor.fetchone()[0] == _csv_row_count(csv_path) |
There was a problem hiding this comment.
As of 8/6/2026 this test is failing with 80 == 81 assertion error
| """, | ||
| ("OCC",), | ||
| ) | ||
| assert cursor.fetchone()[0] == pytest.approx(1789930928.8581762) |
There was a problem hiding this comment.
As of 8/6/2026 this is failing. Cost may have changed and defaults/tests may need to be updated to match
| ) | ||
| rows = {name: (value, unit) for name, value, unit in cursor.fetchall()} | ||
|
|
||
| assert rows["P_DEC"][0] == pytest.approx(50.019897669130195) |
There was a problem hiding this comment.
Same as above, defaults as written seem to give 70 rather than 50
| ) | ||
| rows = cursor.fetchall() | ||
| assert rows[0][0] == "P_DEC" | ||
| assert rows[0][1] == pytest.approx(50.019897669130195) |
There was a problem hiding this comment.
P_DEC difference between test and default outputs
There was a problem hiding this comment.
will update the test value with the new default
|
Seems forgot to update the test after the new value |
| "", | ||
| "no_vpumps", | ||
| ) | ||
| assert rows["no_vpumps"] == ( |
There was a problem hiding this comment.
Significant difference in this test. Test value is 42.25 and new value is 2.26. Was there a large change in either a_EC or some difference in definition of L that caused a change here? It seems to me like 2.26 is too small but I haven't looked into the physics
| ) | ||
| rows = {row[0]: row[1:] for row in cursor.fetchall()} | ||
|
|
||
| assert rows["HF_magnet_cost"] == ( |
There was a problem hiding this comment.
Error here due to new version not having sc_mat_scale
| ("2211",), | ||
| ) | ||
| total_cost, alg_name, variables = cursor.fetchone() | ||
| assert total_cost / 1e6 == pytest.approx(446.188406030979) |
There was a problem hiding this comment.
test seems to get 0.51 vs the 446.19 expected. This is another large change, makes me wonder if one of the lengths is messed up (L_CC or L)
| ("2212",), | ||
| ) | ||
| total_cost, variables = cursor.fetchone() | ||
| assert total_cost / 1e6 == pytest.approx(41.34793800403872) |
There was a problem hiding this comment.
Looks like a difference in EC cost again. Worth mentioning that some of the variables that are constants within the code were changed as part of edits to TEAm when I was running final versions for the first draft. I may want to re-check some of these, especially some of the radii/length definitions for the final version
|
Hi Matt @NybergWISC Thanks for catching the value issue. I updated both the ACCERT SQLite database and the SQL schema/data dump so they now use the new confirmed Mirror baseline values. |
There was a problem hiding this comment.
@NybergWISC This file will be deleted after review; put it here for easy comparison.
2 participants
Merge Request Description
This PR cleans up the Mirror model integration and prepares it for merge into the SQLite-backed ACCERT workflow.
Key updates:
MirrorFunc.py.MirrorFunc.py.tutorial/accert/Mirror.sonto explicitly use the same numeric baseline setup as Matt’srunFOAK.pycase.For Change Request Review