Fix multi-commodity transactions, add preferences #10
Conversation
abachma2
added
Comp:Build
|
@abachma2 is this ready for review or still in progress? |
|
Yes. This is ready for review. |
examples/simple.xml
Outdated
| <!--entry> | ||
| <number>1</number> | ||
| <prototype>TwoReactor</prototype> | ||
| </entry--> |
There was a problem hiding this comment.
How come this is commented out?
There was a problem hiding this comment.
I am benchmarking a lot of the functionality against the Cycamore Reactor archetype, so this was an easy way for me to switch between running the simulation with the OpenMCyclus and Cycamore archetypes. I will remove this section because I ended up creating another file that runs with the Cycamore archetype, so this is no longer needed.
openmcyclus/DepleteReactor.py
Outdated
| refuel_time = ts.Int( | ||
| doc = "Time steps for refueling", | ||
| tooltip="Time steps for refueling", | ||
| uilabel="refueltime", | ||
| default = 0 | ||
| ) | ||
|
|
||
| n_assem_core = ts.Int( | ||
| doc = "Number of assemblies in a core", | ||
| tooltip = "Number of assemblies in a core", | ||
| uilabel = "n_assem_core", | ||
| default=0 | ||
| ) |
There was a problem hiding this comment.
Spacing is inconsistent throughout this section, minor issue. Consider using autopep8 to fix it, but not necessary change.
openmcyclus/DepleteReactor.py
Outdated
| print("time:", self.context.time, "tick") | ||
| if self.retired(): | ||
| print("time:", self.context.time, "retired") | ||
| # self.record("RETIRED", "") |
There was a problem hiding this comment.
The comments for the self.record method are intended to be eventual functionality to record to the database when certain things happen, such as starting a new cycle or retiring a reactor.
|
Thanks for the review @LukeSeifert. I'll work on making the documentation better. I have made some of the other changes requested. |
|
I have found a bug in the feature added through this PR. The requested material is based more on their ordering in the input file than the defined preferences. This results in errors in how much of the material is accepted if there is not enough to meet a full request. I am working on fixing this bug. |
Using a default of 0 gave an error from cyclus that this would be deprecated (soon? maybe it already is?). Changed to one to provide a consistent value that will be accepted by cyclus
|
I have updated the doc strings to be more comprehensive. I have also updated the bug for the preferences described in the previous comment. The bug was because of a lack of feature in the Python API in cyclus, so that has been added in. 4/6 tests are passing locally, with the two resources tests not passing. This is already documented in issue #9. |
openmcyclus/DepleteReactor.py
Outdated
| for ii in range(len(trades)): | ||
| print("time:", self.context.time, "number of trades:", len(trades)) | ||
| print(trades[ii]) |
There was a problem hiding this comment.
Consider adding more detail (such as print(f'Trade {ii}: {trades[ii]}') or something similar) so the information is easier to read
openmcyclus/DepleteReactor.py
Outdated
| commodity = trades[ii].request.commodity | ||
| mat = mats[commodity].pop(-1) | ||
| responses[trades[ii]] = mat | ||
| self.resource_indexes.pop(mat.obj_id) | ||
| self.push_spent(mats) | ||
| print(responses) |
There was a problem hiding this comment.
Consider adding more detail here as well
openmcyclus/DepleteReactor.py
Outdated
| ''' | ||
| n_load = min(len(responses), self.n_assem_core - self.core.count) | ||
| print("time:", self.context.time, "responses:", responses) | ||
| # min(len(responses), self.n_assem_core - self.core.count) |
openmcyclus/DepleteReactor.py
Outdated
| for trade in responses: | ||
| print(trade.request.commodity, trade.request.preference, trade.amt) |
There was a problem hiding this comment.
Consider adding more detail to this print statement
|
Thanks for the review. Those print statements were meant for debugging for development and aren't needed in the final version. I'll remove them. |
|
The extra print statements have been removed and pep8 formatting has been enforced. |
This PR has two primary additions:
Test and test-related files have been updated to test the new functionality and ensure the bug is fixed. There are two tests that are failing because of a bug with resource IDs for the spent fuel. This bug (Issue #9) will be fixed in a later PR. The 2 tests regarding transactions are both passing, and those are the tests for the feature and bug fix in this PR.