-
Notifications
You must be signed in to change notification settings - Fork 87
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/heatr moreio #284
Fix/heatr moreio #284
Conversation
@kahlerac Can you have a look at this one? |
Code changes look good, but for heatr at the start of subroutine hconvr there is a "allocate(scr(npage+50)) statement. With the new "moreio" calls later in this subroutine this allocation likely isn't large enough. |
@kahlerac Good point. By what amount should we increase this? I did see a lot of npage+50 comparisons in heatr so we may want to replace that with a single variable so that its easier to increase later on. |
Yes, this is one of those frustrating situations where today's big enough
array will at some future date become too small. The most secure (and
complex) answer would be to break the initial list or tab read into pieces
... with a "CONT" for the first six words and now "N2" tells us what we
really need for the subsequent array. But that would require a lot of code
changes/verification. As written now "npage+50" is only guaranteed to
allow enough space for a single list or tab read and any subsequent
"moreio" with an increasing array index risks an array overflow. The more
expedient answer is to pick a best guess for "big enough". I'd think we
could get by with a few thousand words, but maybe go for 10K or more and
that should defer the issue until your successor is in charge!
Skip
…On Wed, Feb 22, 2023 at 10:49 AM Wim Haeck ***@***.***> wrote:
Code changes look good, but for heatr at the start of subroutine hconvr
there is a "allocate(scr(npage+50)) statement. With the new "moreio" calls
later in this subroutine this allocation likely isn't large enough.
Good point. By what amount should we increase this? I did see a lot of
npage+50 comparisons in heatr so we may want to replace that with a single
variable so that its easier to increase later on.
—
Reply to this email directly, view it on GitHub
<#284 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEHJISLDDEJK75L3WDLCGPDWYYYP7ANCNFSM6AAAAAAU6PLJ6Y>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
Dr. A. C. (Skip) Kahler
Kahler Nuclear Data Services, LLC
***@***.***
+1 321 368 3645
|
@kahlerac I checked other modules in NJOY2016 and they all use npage+50 when allocating the scr array that read from the ENDF tape. We should talk about what to do here during our next meeting - which I'll try to attend (I promise). |
I'm merging this in develop since I need it in there. @kahlerac We'll talk about the npage+50 thing on a separate occasion. |
A small update to fix issue #283
The ENDF read routines do not read the entirety of TAB1 and LIST records by default. To do that, you need to do a "do while call moreio" after calling the tab1io and listio functions. In some modules this is not done rigorously - probably because it was assumed that the data was read in entirely anyway (e.g. Legendre coefficients were limited to just a few so no need to call moreio or tabulated mutliplicities are just a few values). While these assumptions have hold up, recent evaluations have started to break this. GROUPR was corrected in NJOY2016.69 for this very issue.
This time, it is HEATR's turn because newer evaluations use tabulated multiplicities that are quite large and the tab1io routine could not read these in a single pass.
In addition to this fix, I went down the rabbit hole and removed a few unused variables from acepn.f90.
NJOY2016 version number is set to 2016.70 and release notes were created for this future version (to be released around the end of March 2023).