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

[DO NOT MERGE] Demonstrate a method for handling ciso terms in C truncation #900

Closed
wants to merge 1 commit into from

Conversation

billsacks
Copy link
Member

This PR is for discussion only. When @negin513 was trying to fix #811 , she ran into problems which I guess point out why the code referenced in #811 was done that way in the first place. The basic problem is that the c13 / c14 arrays aren't always allocated, and so we can't use the standard method for passing sub-arrays into subroutines (e.g., foo(bounds%begp:bounds%endp)).

My first thought was that we could define variables like c13_begp and c13_endp; these would be set equal to bounds%begp and bounds%endp if use_c13 is true, and 0 otherwise. So if use_c13 is false, we would pass the slice foo(0:0). But I'm not sure if that will work on all compilers (or any compiler, for that matter).

Then I thought about how I am handling things like this with water isotopes: Basically, first I do some operation on the bulk, possibly returning a logical array or filter specifying the points that need to be operated on for each isotope. Then I handle the isotopes in a separate call, passing in the already-computed logical array or filter. This is what I have done here.

I don't love the extra code needed for each call to TruncateCandNStates, but I do like that: (1) it allows us to handle the c13 and c14 arrays like everything else in CTSM, including bounds checking; (2) it doesn't require c13 and c14 arrays to be pointers (important if we ever want to tackle #235 ); (3) it cleans up the body of TruncateCandNStates.

This is only partially implemented and won't compile yet. Needs are:

(1) Changing all of the calls to TruncateCandNStates to use the new pattern

(2) Applying the same pattern to TruncateCStates - in both the implementation of this routine and all calls to it

But before I went further with this, I wanted to run it by @ekluzek and @negin513 . Does this seem like a good solution to both of you? If so, I'd ask @negin513 to finish the implementation.

@ekluzek - I'm a little nervous about rewriting this module like this without knowing if this code is actually triggered typically. Erik, do you know if this code is triggered in typical runs? In particular, can we expect it to be triggered in our ciso tests? Ideally, I'd want a test that triggers truncation for at least one point for each and every call to TruncateCandNStates and TruncateCStates to ensure that all of the code has been refactored properly. That may not be feasible, but I'd at least hope for one variable that triggers truncation in one of the ciso tests; then that test demonstrating bit-for-bit (which would show that the basic logic in TruncateCandNStates is good) together with a very careful code review (focusing on whether the correct variables are used for each call to TruncateCandNStates and TruncateCStates) could give me enough confidence that the refactoring was done properly.

@negin513 if @ekluzek doesn't know the answer to that question, then I'll ask you to investigate which of these calls actually have truncation occurring in our typical ciso tests. If none do, then we should either run a long test (e.g., 10-year f10 case, if that triggers these) with comparison against baselines, or somehow force some of these truncations to be triggered.

This is only partially implemented and won't compile yet. Needs are:

(1) Changing all of the calls to TruncateCandNStates to use the new
    pattern

(2) Applying the same pattern to TruncateCStates - in both the
    implementation of this routine and all calls to it
@billsacks billsacks added the PR status: work in progress PR: author feels this is NOT ready to merge to master label Jan 30, 2020
@billsacks
Copy link
Member Author

Closing, as this has been incorporated in #923

@billsacks billsacks closed this Feb 11, 2020
@billsacks billsacks deleted the truncate_ciso branch February 11, 2020 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR status: work in progress PR: author feels this is NOT ready to merge to master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some bounds assertions aren't done when threading is on
1 participant