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

EAMxx: intelligently handle fillval tracking in diags #6803

Open
mahf708 opened this issue Dec 5, 2024 · 9 comments
Open

EAMxx: intelligently handle fillval tracking in diags #6803

mahf708 opened this issue Dec 5, 2024 · 9 comments
Assignees
Labels
EAMxx PRs focused on capabilities for EAMxx help wanted

Comments

@mahf708
Copy link
Contributor

mahf708 commented Dec 5, 2024

Diagnostics being their own derived classes are supposed to handle variety of inputs/outputs.

  • Sometimes diagnostics can produce their own fillval depending on conditions. Take FieldAtXyz being unavailable on a mountain or something. Another is OpticalDepth not being available at night.
  • Other time, diagnostics must take into them masked fields, and thus will have to reckon with these masked/fill values.

In both cases, diagnostics are supposed to be able to cleanly and intelligently handle accumulation. Currently, only the FieldAtXyz is handled okay.

Options:

  1. Make all diagnostics redundantly track accumulation. This is not desirable as it is a blanket solution
  2. Introduce a functionality to determine if a diagnostic needs to track the accumulation carefully based on the conditions above (i.e., it either takes in a field that requires careful handling or that it itself produces fillvals thus needing careful handling)
@mahf708 mahf708 added help wanted EAMxx PRs focused on capabilities for EAMxx labels Dec 5, 2024
@bartgol
Copy link
Contributor

bartgol commented Dec 5, 2024

What do you mean with "track accumulation"? Do you mean track averaging tally? I don't think diags are the right place to handle averaging. An output stream may have a mix of FM fields and diagnostics, so having diags do the tally while FM fields don't would make IO even more complex. Besides, the avg count is the same for all "field_xyz_at_1000hPa", so IO is able to only allocate one field for the avg counting.

I think we can make diags tell IO whether they may generate mask/fill values. Some diags may do it directly (like FieldAtHeight), while others may do it indirectly. E.g., in aodvis_at_modeltop, the "outer" diag is FieldAtLevel, which does not per se generate Fill/Mask values, but the overall output may, since the inner diag does generate them.

I propose adding to AtmosphereDiagnostic an interface like

virtual void can_generate_invalid_values () const { return false; }

Being false by default, all diags that do not generate invalid values don't need to be modified. But FieldAtHeight or AodVis can return true. IO will then take care of checking if a diag can generate invalid values "indirectly". E.g., like this

   bool can_gen_fill_val = diag->can_generate_invalid_values();
   for (const auto& dep : m_diag_depends_on_diag[diag->name()]) {
      can_gen_fill_val ||= m_diagnostics.at(dep)->can_generate_invalid_values()
   }

@mahf708
Copy link
Contributor Author

mahf708 commented Dec 5, 2024

I think we can make diags tell IO whether they may generate mask/fill values.

Yes, this is what I meant by "handle fillval" / "track accumulation" --- tell the appropriate infra to do it (not actually do it in diags)

@mahf708
Copy link
Contributor Author

mahf708 commented Dec 5, 2024

Being false by default, all diags that do not generate invalid values don't need to be modified. But FieldAtHeight or AodVis can return true. IO will then take care of checking if a diag can generate invalid values "indirectly". E.g., like this

Yes, but we also need to handle the stuff it's receiving ... maybe:

virtual void can_generate_invalid_values () const { 
  bool some_bool;
  for (const auto& f : get_fields_in()){
    some_bool ||= f.has_mask();
  }
  return some_bool;
}

then override inside diag if we are sure it may make invalid values itself, etc.

@mahf708
Copy link
Contributor Author

mahf708 commented Dec 5, 2024

I will get a PR done soon (likely over the weekend)

@bartgol
Copy link
Contributor

bartgol commented Dec 5, 2024

Being false by default, all diags that do not generate invalid values don't need to be modified. But FieldAtHeight or AodVis can return true. IO will then take care of checking if a diag can generate invalid values "indirectly". E.g., like this

Yes, but we also need to handle the stuff it's receiving ... maybe:

virtual void can_generate_invalid_values () const { 
  bool some_bool;
  for (const auto& f : get_fields_in()){
    some_bool ||= f.has_mask();
  }
  return some_bool;
}

then override inside diag if we are sure it may make invalid values itself, etc.

I don't think so. If an input field X is from the FM, then it's out of the diag responsibility. It is no different than outputing X directly. If we want to handle FM fields that MAY be masked/filled, then we need to also do that check on the metadata for every single field in the Output class...

@mahf708
Copy link
Contributor Author

mahf708 commented Dec 5, 2024

But what if we get field X into the diag and then do some ops on it (like say X_at_model_top). Assuming X is masked at that instance, then X_at_model_top will return a masked value, no? How would we deal with that?

@bartgol
Copy link
Contributor

bartgol commented Dec 6, 2024

Do we have FM fields that can be masked/filled? I don't think we currently have any. But maybe I'm forgetting some

@mahf708
Copy link
Contributor Author

mahf708 commented Dec 6, 2024

That's a good question. I assumed we had some fields like that. I now think we actually don't ... see this search query of our codebase

@mahf708
Copy link
Contributor Author

mahf708 commented Dec 6, 2024

This code in COSP should be replaced with a proper masked value treatment: link. That would be the first application of it I guess ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EAMxx PRs focused on capabilities for EAMxx help wanted
Projects
None yet
Development

No branches or pull requests

3 participants