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

Feature/upn epm #160

Merged
merged 12 commits into from
May 6, 2020
Merged

Feature/upn epm #160

merged 12 commits into from
May 6, 2020

Conversation

kahlerac
Copy link
Contributor

Adding, and changing some existing, group identifiers in ERRORR and GROUPR.
Share code for uniform group labels in both GROUPR and ERRORR.

@jchsublet Will this affect your legacy NJOY2012 inputs? We're changing group numbers from the local upn created for the Agency several years ago.

Copy link
Member

@jlconlin jlconlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So overall, I think this looks good. It simplifies the input by making the group structures between ERRORR and GROUPR the same. It also adds a number of new group structures.

We will need to make a complementary Pull Request on the NJOY2016 manual.

!-------------------------------------------------------------------
use mainio ! provides nsyso
use util ! provides error
use groupm ! provides gengpn
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how we are now using the same thing in both ERRORR and GROUPR. No need to put this in twice!

! 18 xmas 172-group structure
! 19 read in, supplemented with endf covariance grid
! 18 xmas nea-lanl
! 19 ecco 33-group structure
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to mess up someone's input. If they update and re-run their calculation, they are going to get different answers. Do we want to avoid using 19 to preserve existing capabilities?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whenever 19 is used, NJOY2016 will write a message out suggesting changing it to -1. In case the input still assumes the old 19 value, it will crash eventually because it'll try to interpret lines in the input file as the user defined structure.

@@ -2471,6 +2498,1564 @@ subroutine gengpn
1.252320e+07_kr,1.284030e+07_kr,1.349860e+07_kr,1.384030e+07_kr,&
1.419070e+07_kr,1.454990e+07_kr,1.491820e+07_kr,1.568310e+07_kr,&
1.648720e+07_kr,1.690460e+07_kr,1.733250e+07_kr,1.964030e+07_kr/)
real(kr),parameter::eg24(282)=(/&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to assume that these are the same values that were in error.f90.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, these were the same

@jlconlin
Copy link
Member

PS. I've run the tests on my machine and they all pass. Let's wait and see what GitHub Actions reports.

@jchsublet
Copy link

@wim @jlconlin @kahlerac
Groups identifiers are extremely important in groupr, why they differs in errorr is for me a mystery, errorr should have a subset, and only a subset with the same read in id. GROUPR is thousand times more important than errorr for useful applications, if you insist to change ERRORR logic do in such a way that it correspond to GROUPR, the influence it will have on legacy input will be minimal and to be honest errorr and errors users are not legion.

Copy link
Member

@whaeck whaeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I wrote most of this so ...

Copy link
Member

@jlconlin jlconlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last thing I'd like to see before we publish this change. Can someone (@kahlerac or @whaeck) please add some words to the ReleaseNotes?

@jlconlin jlconlin merged commit d5ea3f1 into master May 6, 2020
@whaeck whaeck deleted the feature/upn-epm branch March 9, 2021 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants