-
Notifications
You must be signed in to change notification settings - Fork 3
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
Initial Release #1
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me overall. There are a few definitions that we should check with people who know more about fusion systems generally and comments/tooltips to clean up but great job getting version 0 up and running so quickly!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted a few administrative things like defaults and extra comments left in the code, and made a few suggestions about how to use uitypes to restrict the values your users can submit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a first pass. Definitely should have been talking about the meat of your modeling choices, but maybe you've been doing that with others?
/// Place a description of the detailed behavior of the agent. Consider | ||
/// describing the behavior at the tick and tock as well as the behavior | ||
/// upon sending and receiving materials and messages. | ||
class Reactor : public cyclus::Facility { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want to call this a FusionPowerPlant
just to be clear that it's different from a fission Reactor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bunch of comments on code readability here.
Co-authored-by: Katie Mummah <[email protected]>
Putting this before your ascii art will make it format correctly, I think:
It might also work to put a triple back tick before/after: ``` |
…iate changes to unit tests, cleaned up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A number of additional tips... you're reinventing the wheel in a few places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More thoughts
Co-authored-by: Paul Wilson <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one simplifying suggestion for a conditional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely looking cleaner and more readable! A few more quick comments while you are in the middle of edits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
continued march towards a simpler chemistry math - all-in on moles
cyclus::Facility::EnterNotify(); | ||
|
||
fuel_usage_mass = (burn_rate * (fusion_power / MW_to_GW) / seconds_per_year * context()->dt()); | ||
fuel_usage_atoms = fuel_usage_mass / tritium_atomic_mass; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since you've switched to moles, why not make this moles instead of atoms, and all of your atomic masses can be molar masses....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that the code is more readable, we should revisit your modeling choices and function names.
#include "reactor.h" | ||
|
||
#include "boost/shared_ptr.hpp" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider including some of these to reduce the need to fully specify the namespace of frequently used cyclus classes:
using cyclus::Material;
using cyclus::Composition;
using cyclus::toolkit::ResBuf;
using cyclus::toolkit::MatVec;
using cyclus::KeyError;
using cyclus::ValueError;
using cyclus::Request;
using cyclus::CompMap;
fuel_usage_atoms = fuel_usage_mass / tritium_atomic_mass; | ||
blanket_turnover = blanket_size * blanket_turnover_rate; | ||
|
||
fuel_startup_policy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should discuss all these policies at a fuel cycle meeting with @nuclearkatie present. My hunch is that there is an easier way to accomplish what you want.
if (!tritium_storage.empty() && sufficient_tritium_for_operation) { | ||
double surplus = std::max( | ||
tritium_storage.quantity() - reserve_inventory, 0.0); | ||
|
||
if (surplus > 0.0) { | ||
|
||
tritium_excess.Push(tritium_storage.Pop(surplus)); | ||
CombineInventory(tritium_excess); | ||
|
||
RecordOperationalInfo( | ||
"Tritium Moved", | ||
std::to_string(surplus) + "kg of T moved from storage to excess"); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe put this in a function called StoreExcessTritium()
for readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, the next block could be StoreBlanketTritium()
or something like that??
src/reactor.cc
Outdated
blanket_mat = cyclus::Material::Create(this, new_mass, | ||
cyclus::Composition::CreateFromAtom(depleted_comp)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is cyclus-dangerous since your old material just vaporizes and you create a new one from nothing here with a different id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a bad feeling that this was illegal, but it seemed like the most straightforward way to get this to work. I'll come up with a different way to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should look at what the RecipeReator does in Cycamore. I may be overly cautious here, but it seems like it will be hard to do any material accountability if materials just cease to exist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually figured out a way to just not do this and have everything still work. Fairly certain this version isn't cyclus-dangerous, but I'll leave discussing it in the fuel cycle meeting notes for today, and maybe we can have a broader discussion about the finer points of not doing this (I think I get the broad strokes of what isn't allowed for the most part, especially after you pointed this out)
DepleteBlanket(atoms_burned/avagadros_number * TBR); | ||
cyclus::Material::Ptr mat = blanket.Pop(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last thing that DepleteBlanket()
does is push a material onto the buffer and then the first thing you do here is pop it off the buffer again. Maybe DepleteBlanket()
should just return the material ptr?
cyclus::compmath::Normalize(&c, 1); | ||
|
||
if (tritium_storage.quantity() < startup_inventory){ | ||
throw cyclus::ValueError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you actually want this to throw a ValueError and break the simulation, or just not start up at this point?
Add github action to test build
No description provided.