Allow for tracer to not be advected by SHOC #3085
Conversation
| "turbulence_advected_tracers") != req.groups.end(); | ||
| ss << " - (" + req.calling_process + ", " + grid_name + ", " + (turb_advect ? "true" : "false") + ")\n"; | ||
| } | ||
| EKAT_ERROR_MSG(ss.str()); |
There was a problem hiding this comment.
Sample output in case that P3 requests qv to not be advected by SHOC.
Error! Incompatible tracer request. Turbulence advection requests not
consistent among processes.
- Tracer name: qv
- Requests (process name, grid name, is tracers turbulence advected):
- (homme, Physics GLL, true)
- (mam4_aci, Physics GLL, true)
- (p3, Physics GLL, false)
- (homme, Physics GLL, true)
There was a problem hiding this comment.
Why is there a repeated entry for homme?
There was a problem hiding this comment.
I think it is because there exists a computed and required group request (since "qv" is "updated" in homme), but then I don't know why p3 does not have 2 requests as well. Let me investigate.
There was a problem hiding this comment.
So yes, double entries are due to "updated" fields, since a request exists for "required" and "computed".
Also, field requests in ATMProcGroup is a set, and so only one version of each reqest is used. By chance homme's "qv" was the version of "qv" with packsize>1, P3 was picked up since it's req.groups was unique (i.e., no turbulence advection), and mam4_aci was picked up as the packsize=1 request (all other mam "qv" request ignored.
@bartgol I've pushed a solution that does the following
- Makes
tracer_requestsa map of set of field requests, so removes duplicate computed/required entry. - Use
req.calling_processinFieldRequest::operator<so that requests with different calling_process are distinct (when loaded into astd::set<FieldRequest>). Them_*_field_requestsinAtmosphereProcessis now larger, but it does not result in more allocations since FM will skip extra requests when it allocates.
The other options for 2. I think are
a. remove all calling process info in FieldRequest and simplify the error message with no calling process and just say "qv has incompatible group requests" and make the user find the processes,
b. or leave as it was in the output above which is not complete but gives user the name of conflicting processes.
There was a problem hiding this comment.
Maybe we could put an enum in the request, that specifies whether this req is for input, output, or input-output. It may be helpful for debugging purposes.
That said, I don't think that seeing duplicated processes in the output is a big deal. What I would do, maybe, is explain why the requests are incompatible. That is, you could add a line at the bottom of the error msg, like
Error! Incompatible tracer request. Turbulence advection requests not
consistent among processes.
- Tracer name: qv
- Requests (process name, grid name, is tracers turbulence advected):
- (homme, Physics GLL, true)
- (mam4_aci, Physics GLL, true)
- (p3, Physics GLL, false)
- (homme, Physics GLL, true)
All processes MUST agree on whether this tracer is advected by the turbulence scheme.
components/eamxx/src/physics/mam/eamxx_mam_microphysics_process_interface.cpp
Outdated
Show resolved
Hide resolved
components/eamxx/src/physics/mam/eamxx_mam_microphysics_process_interface.hpp
Outdated
Show resolved
Hide resolved
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Using Repos:
Pull Request Author: tcclevenger |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
SCREAM_PullRequest_Autotester_Weaver # 6267 FAILED (click to see last 100 lines of console output) |
tcclevenger
force-pushed
the
tcclevenger/add_tracer_with_turbulence_advection
branch
from
14a4fd7 to
55b1441
Compare
tcclevenger
changed the title
tcclevenger
changed the title
|
@singhbalwinder @odiazib Any thoughts on these variables ( |
| "turbulence_advected_tracers") != req.groups.end(); | ||
| ss << " - (" + req.calling_process + ", " + grid_name + ", " + (turb_advect ? "true" : "false") + ")\n"; | ||
| } | ||
| EKAT_ERROR_MSG(ss.str()); |
There was a problem hiding this comment.
Why is there a repeated entry for homme?
| @@ -534,6 +534,66 @@ void AtmosphereDriver::create_fields() | |||
| m_field_mgrs[grid->name()]->registration_begins(); | |||
| } | |||
|
|
|||
| // Before registering fields, check that Field Requests for tracers are compatible | |||
| { | |||
There was a problem hiding this comment.
I wonder if this snippet should be moved somewhere, like in share/util, or maybe as a free function in field_request.hpp, something like
// ensure all requests are on the same page on whether the field belongs to $group_name
bool compatible_groups (std::vector<FieldRequest>& reqs, std::string& group_name);(maybe with a better name)
|
@tcclevenger @singhbalwinder @odiazib any thought on the eamxx-sa fails? They're all baseline_cmp for mam tests. Was shoc advecting ALL tracers before, so we were doing the wrong thing before and the right thing now? |
|
I expected the MAM4xx tests to fail as SHOC was originally advecting (vertical mixing) the interstitial aerosols, but now it is not (as far as I understood). This should cause all such tests to fail. |
|
I correct myself: the FPE shows an error for the |
|
Yeah, let me run the FPE test, it may be related to the CIME case error I posted above. |
Here are the fields in each tracer group (I output at SHOC and HOMME So, @singhbalwinder Do these groups look right in terms of interstitial and gas aerosols? |
|
Looks great! Yes, the species are grouped correctly. |
|
@singhbalwinder correct on the tracer names. I've narrowed down the issues in the FPE tests and the |
tcclevenger
force-pushed
the
tcclevenger/add_tracer_with_turbulence_advection
branch
from
55b1441 to
713aa2d
Compare
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🟠 Enforce checks passingWaiting checks:
|
|
@tcclevenger and @bartgol : I have added a fix for the failing tests. Can one of you trigger the tests again to see if the fix fixes all the issues? Thanks! |
|
@singhbalwinder seems your commit fixed the FPE fail, but not the CIME case fail (where rrtmgp aerosols blow up). I was hopeful they would be related. I won't be able to look at this until next week, but I can put it at the top of my list. |
|
@tcclevenger: I will look into the CIME case as well to see if I can fix this error. |
Increases the number of entries in the AtmosphereProcess set m_*_field_requests, but not the number of allocations. Allows us to output all instances of field requests of the same name.
tcclevenger
force-pushed
the
tcclevenger/add_tracer_with_turbulence_advection
branch
from
06d1637 to
457bc78
Compare
|
Update: The issue currently is that for PG2, homme interface is adding tracers on the GLL grid as an "import" group, but that group does not have the same order of tracers. So when homme is remapping, from gll -> pg2, the values do not match the tracer names. |
Ah, that's annoying. Indeed, the tracers are re-ordered when the FM allocates the fields, while the imported group is setup after requests have been gathered but before the FMs allocates the fields... @tcclevenger maybe we should handle grids in such a way that:
This makes me really want to get rid of having multiple FM, and store ALL fields in the same FM... |
|
Moved to E3SM-Project/E3SM#6789 |
Changes here are very simple: if we want tracer advected by SHOC,
add_tracer()adds to group "turbulence_advected_tracers" and "tracers", if not, only add to "tracers". Almost all the complexity of the PR comes from the error message when we find conflicting tracer requests wrt. SHOC advected.Name `turbulence_advected_tracers" is not final, will consult with others before merging.
@bartgol I opted against an enum for
turbulence_advectedwhere we have a "don't care" option. I think it makes it more confusing, and processes that want this represent more of a special case. Maybe that is an argument for defaulting this value to true? Feel free to push back though.