-
Notifications
You must be signed in to change notification settings - Fork 5
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
Corrected density for lithium ceramic and added potential solid blank… #12
base: main
Are you sure you want to change the base?
Conversation
I'd suggest including the old and new densities here, as well as why you think the new value is more correct. |
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.
Li2TiO3nat.density = 3.43 # This migh of been always wrong need to check- it was :(
This is the correct mass density according to the citation, but Li2TiO3Li60 had the incorrect mass density (suggest deleting the comment)
reference: Van Der Laan, J., Reimann, J., & Fedorov, A. (2016). Ceramic Breeder Materials. In Elsevier eBooks (pp. 114–175) https://doi.org/10.1016/b978-0-12-803581-8.00733-5
Following the existing convention in the python script, the first time a reference appears, it should use the string fullreference to make parsing the python script easier to generate the citation database and it should all appear on one line with the "citation metadata" identifier string constructed by FirstauthorlastnameJournal_year, so it would be:
fullreference: VanderLaanCeramicBreederMaterials_2016 Van Der Laan, J., Reimann, J., & Fedorov, A. (2016). Ceramic Breeder Materials. In Elsevier eBooks (pp. 114–175) https://doi.org/10.1016/b978-0-12-803581-8.00733-5
Similar comment for Be17Ti2 reference (SpringerMaterials)
Similar comment for Be17Ta2 reference
Similar comment for Be12Ta reference
Similar comment for Be12Fe reference
Functions for Be12Ta, Be12Fe have atoms of Be as 13 rather than 12
Functions for defining several Pb compounds seem to use the string Be12Fe as the material object name. It would be less confusing to read if we used the material name e.g., LaPb3, YPb3, YPb2, Zr5Pb4, Zr5Pb3
mat_lib["Be12Fe"] = Be12Ta_mat()
should call Be12Fe_mat()
Some of the citations only include one of the references, should I update them to include both as part of their metadata? Also as part of the review function, I included a comment about how "reference:" has different formatting and I was wondering if you want me to make it the same throughout. And if so what style do you want for that? Or do we even need a reference for every entry they all follow the same as the above reference in the case for the Li ceramics cites by HernandezFusEngDes_2018? |
A perfect example of why we should refactor some of this code to avoid hard to notice mistakes like this. |
Given the other changes and based on discussions at our Tuesday afternoon neutronics meeting, I propose we close this pull request and then make changes following the workflow of the other updates that have been made. At that point issue a new PR. The main things to keep from the old PR are:
|
Can this be rebased and/or reimplemented against recent PEP8 and directory changes? |
That's the plan. I've just been waiting for things to settle down. |
Great -- I was just looking through my PRs and making sure I knew where things stood 👍 |
Added new materials: Beryllium and Lead neutron multipliers and new Lithium materials. Formated the code with Black formatted. Corrected the density for Li2Ti03Li60 from 3.42 to 3.43, so that it would match Li2Ti03nat density and the density stated in the source referenced. When estimating what the new density would be at an enrichment of Li-6 to 60%, I calculated 3.40 g/ cm3. I think it's best to keep the theoretical density the same for the enriched and natural lithium cases for now.