Skip to content
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

Fix for NDI AMW usage #771

Merged
merged 2 commits into from
Mar 23, 2020
Merged

Fix for NDI AMW usage #771

merged 2 commits into from
Mar 23, 2020

Conversation

brryan
Copy link
Contributor

@brryan brryan commented Mar 23, 2020

Background

  • Passing zaid=2004 to NDI_AtomicMass would cause NDI to return 7 AMWs (for zaid=2004 and other zaids like 20040)
  • Tom Saller showed me how to fix this: append a "." to the zaid.

Purpose of Pull Request

Description of changes

  • Reformat the ZAID when converting from an integer before passing to NDI.

Status

@brryan brryan added the bug label Mar 23, 2020
@brryan brryan added this to the Draco-7_7_0 milestone Mar 23, 2020
@brryan brryan requested a review from keadyk March 23, 2020 15:39
@brryan brryan self-assigned this Mar 23, 2020
@codecov
Copy link

codecov bot commented Mar 23, 2020

Codecov Report

Merging #771 into develop will increase coverage by 0.0%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           develop    #771   +/-   ##
=======================================
  Coverage     94.0%   94.0%           
=======================================
  Files          367     367           
  Lines        17227   17230    +3     
=======================================
+ Hits         16195   16198    +3     
  Misses        1032    1032           

@keadyk
Copy link
Collaborator

keadyk commented Mar 23, 2020

LGTM! I'll wait a little longer to see if anything comes in from tt, but I'll merge soon.

@KineticTheory the Cdash coverage build for this PR looks really weird. There's no way this PR could've caused coverage to drop by ~7%, so I'm going to ignore it... it looks like maybe the test directories are getting included for some reason. The codecov report here on GitHub looks fine!

@KineticTheory
Copy link
Collaborator

Let's wait for tt. It is backlogged with a cta CI job.

@KineticTheory
Copy link
Collaborator

LGTM! I'll wait a little longer to see if anything comes in from tt, but I'll merge soon.

@KineticTheory the Cdash coverage build for this PR looks really weird. There's no way this PR could've caused coverage to drop by ~7%, so I'm going to ignore it... it looks like maybe the test directories are getting included for some reason. The codecov report here on GitHub looks fine!

Yeah. I was looking at that. It seems that @brryan's branch is a bit out-of-date and doesn't have the exclusions for code in test directories and STL headers that I introduced in #751 and updated in #766.

@KineticTheory
Copy link
Collaborator

I don't consider the coverage issue to be a blocker. It should merge cleanly.

@KineticTheory
Copy link
Collaborator

BTW - the trinitite CI for this PR is next in line and should start within 15 minutes.

@keadyk
Copy link
Collaborator

keadyk commented Mar 23, 2020

tt passing! Merging now :)

@keadyk keadyk merged commit c5d68a4 into lanl:develop Mar 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants