Update material compositions #14
Conversation
Need to figure out way to convert ZAIDs to nucnames (e.g. U235). Also need to figure out a way to determine whether to use atom fraction or weight fraction based on cyclus input definition.
The naming required is for the fuel-containing materials to be called assembly_#. Th enew geometry is three spheres surrounded by water, all contained in a larger sphere. Each smaller sphere is meant to be a different assembly.
The dictionary of compositions provided by Cyclus is always a weight fraction.
OpenMC needs a nucname (e.g., U235) for the nuclide names, these additions convert the ZAID from Cyclus compositions to the nucname for use in OpenMC
The file meant to be read into Cyclus is in the examples directory, not in the test diestory with the OpenMC files. Should change this location in a future commit.
Changed passing the prototype name as a class argument to a method argument because something wasn't being passed correctly when defined as a class argument
|
@samgdotson I have updated the CI to have Cyclus and OpenMC. The integration tests are failing, as expected. It doesn't provide much of an error message as to why, so it doesn't help with information on the seg fault. |
|
Good job updating the CI. Just to clarify, the integration tests pass locally but not on github, correct? The reason the tests show a "table not found" error is because cyclus doesn't run properly? |
|
I believe that's what's happening in the CI, yes. Directly running the tests, they don't pass locally, because the seg fault interrupts the testing before it can compare values. But comparing the output database to the expected values, they do match. |
|
Now that the CI is up to date, is this ready to be merged? |
There was a problem hiding this comment.
It's nearly ready. You should definitely review your code for consistency with numpydoc, overall. I added suggestions where possible. Some suggestions make look insignificant (var: type vs var : type) but it will affect proper formatting if you publish the docs. Also, the environment.yml and requirements.txt files are not the same, which could be problematic for maintenance.
| @@ -3,6 +3,50 @@ channels: | |||
| - conda-forge | |||
| - defaults | |||
| dependencies: | |||
| - numpy | |||
There was a problem hiding this comment.
is numpy really not used in this package?
There was a problem hiding this comment.
Maybe not. I just always add numpy as a force of habit.
| Cyclus. | ||
|
|
||
|
|
||
| Parameters: |
There was a problem hiding this comment.
Numpydoc does not have a : after a header. Just the underscores! See the numpydoc docs.
| Parameters: | |
| Parameters |
| path: str | ||
| path of directory holding the files for/from OpenMC | ||
|
|
||
| Returns: |
There was a problem hiding this comment.
| Returns: | |
| Returns |
| comp_list: list of dicts | ||
| list of the fresh fuel compositions present in the core | ||
| at the calling of the transmute function. | ||
| path: str | ||
| path of directory holding the files for/from OpenMC |
There was a problem hiding this comment.
| comp_list: list of dicts | |
| list of the fresh fuel compositions present in the core | |
| at the calling of the transmute function. | |
| path: str | |
| path of directory holding the files for/from OpenMC | |
| comp_list : list of dicts | |
| List of the fresh fuel compositions present in the core | |
| at the calling of the transmute function. | |
| path : str | |
| Path to the directory holding the files for/from OpenMC |
| material_ids: list of strs | ||
| material id numbers for the OpenMC model |
There was a problem hiding this comment.
Numpydoc style
| material_ids: list of strs | |
| material id numbers for the OpenMC model | |
| material_ids : list of strs | |
| Material identification numbers for the OpenMC model. |
| path: str | ||
| path of directory holding the files for/from OpenMC | ||
|
|
||
| Outputs: | ||
| -------- | ||
| depletion_results.h5: database | ||
| HDF5 data base with the results of the depletion simulation |
There was a problem hiding this comment.
Since the function returns None I would simply leave out the second part of this and describe it in the main part of the docstring. E.g. "writes to an HDF5 database called depletion_results.h5 with the simulation results."
| path: str | |
| path of directory holding the files for/from OpenMC | |
| Outputs: | |
| -------- | |
| depletion_results.h5: database | |
| HDF5 data base with the results of the depletion simulation | |
| path : str | |
| path of directory holding the files for/from OpenMC. |
| Parameters: | ||
| ----------- | ||
| material_id: list of strs | ||
| material ids for the assembly materials in the OpenMC model | ||
| path: str | ||
| path of directory holding the files for/from OpenMC |
There was a problem hiding this comment.
| Parameters: | |
| ----------- | |
| material_id: list of strs | |
| material ids for the assembly materials in the OpenMC model | |
| path: str | |
| path of directory holding the files for/from OpenMC | |
| Parameters | |
| ----------- | |
| material_id : list of str | |
| Material ids for the assembly materials in the OpenMC model. | |
| path : str | |
| Path to the directory holding the files for/from OpenMC. |
|
|
||
| Outputs: | ||
| Returns: |
There was a problem hiding this comment.
| Returns: | |
| Returns |
| spent_comps: list of dicts | ||
| list of the compositions from the OpenMC model |
There was a problem hiding this comment.
dicts isn't a python type, but dict is.
| spent_comps: list of dicts | |
| list of the compositions from the OpenMC model | |
| spent_comps : list of dict | |
| List of the compositions from the OpenMC model. |
| pytest |
There was a problem hiding this comment.
Numpy is in requirements.txt but NOT in environment.yml. Which one is correct? Do you need both files?
There was a problem hiding this comment.
I don't think numpy is needed, so it will be removed. I'm not sure if both files are needed right now. I have gotten the CI into a stable configuration, but definitely not the final one. Things to be done in the CI include installing Cyclus and Cycamore from branches off my forks to use the most updated versions, instead of an out-of-date docker image (or updating the docker image). One I update the builds to the most current versions, I will determine if both of these files are needed.
|
Hi @samgdotson. Thanks for the review and the suggestions to make it easier to publish the docs. I currently have no intentions of publishing the documentation of this repo, especially since most of the methods in the archetype and |
There was a problem hiding this comment.
I don't want to be difficult or obstructive, but I think you should reevaluate the practice of leaving your code littered with commented debug statements. If your code is still in development perhaps this should be merged into a "dev" branch and leave the main branch for the final clean versions.
If you really want this pushed through, then you can request my review again and I will merge it.
|
If the only thing blocking you from merging right now is the print statements, I can remove them in this PR. |
The switch to transmuting only some assemblies was madde in error when resolving a conflict with the main branch
This PR includes:
examples/directory so that all of the example/test files are in the same location.materials.xml)The CI fails because of compatibility issues between Cyclus and OpenMC (see issue #12). CI will be cleaned up some in a future PR.