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

Add a diag_send_complete call #101

Merged
merged 1 commit into from
Feb 13, 2024

Conversation

uramirez8707
Copy link
Contributor

Add a diag_send_complete call. This is needed for the new diag manager. It is backwards compatible with the old diag manager.

@uramirez8707 uramirez8707 marked this pull request as draft February 1, 2024 16:53
@uramirez8707 uramirez8707 marked this pull request as ready for review February 1, 2024 18:55
@bensonr
Copy link
Contributor

bensonr commented Feb 5, 2024

@rem1776 - with Niki's comment about sub-stepping in the ocean component, will this fix be sufficient?

@rem1776
Copy link
Contributor

rem1776 commented Feb 5, 2024

@rem1776 - with Niki's comment about sub-stepping in the ocean component, will this fix be sufficient?

I am not sure. I was under the impression this call would be needed regardless, and other updates would go to the ocean component to handle the sub-stepping if needed.

@uramirez8707 @nikizadehgfdl What do you guys think?

@bensonr
Copy link
Contributor

bensonr commented Feb 5, 2024

@rem1776 - what's the behavior for diag_send_complete when I call it twice in a row with no new send_data calls in between? This is where we might end up with trouble by having it in both the component and the coupler driver.

@uramirez8707
Copy link
Contributor Author

@rem1776 - what's the behavior for diag_send_complete when I call it twice in a row with no new send_data calls in between? This is where we might end up with trouble by having it in both the component and the coupler driver.

That will not be good.
https://github.com/NOAA-GFDL/FMS/blob/bb6de937f70a08a440f5e63b8553b047c1921509/diag_manager/fms_diag_object.F90#L688-L689

@rem1776
Copy link
Contributor

rem1776 commented Feb 5, 2024

@rem1776 - what's the behavior for diag_send_complete when I call it twice in a row with no new send_data calls in between? This is where we might end up with trouble by having it in both the component and the coupler driver.

That will not be good. https://github.com/NOAA-GFDL/FMS/blob/bb6de937f70a08a440f5e63b8553b047c1921509/diag_manager/fms_diag_object.F90#L688-L689

Yeah it'd be incrementing the time step along with running the reduction an extra time.

@uramirez8707
Copy link
Contributor Author

this%current_model_time should be modified inside send_data instead, if there is a new time

@rem1776
Copy link
Contributor

rem1776 commented Feb 5, 2024

this%current_model_time should be modified inside send_data instead, if there is a new time

I could try to add that in and then have it ignore any un-needed reductions from extra calls, but would that solve the issues with the ocean model substeps?

@uramirez8707
Copy link
Contributor Author

This is going to prevent the math from being done twice: NOAA-GFDL/FMS@efbb9f0

The ocean model substeps still need to call diag_send_complete. If they don't the code will crash with something like: https://github.com/NOAA-GFDL/FMS/blob/bb6de937f70a08a440f5e63b8553b047c1921509/diag_manager/fms_diag_file_object.F90#L1204-L1206

@thomas-robinson
Copy link
Member

Is this going to add on to the model time? I don't think the math needs to be done in fms_diag_send_complete in the ocean model because there isn't any threading. The math should all be done in accept_data

@uramirez8707
Copy link
Contributor Author

no it doesn't add to the model time. It just updates the model_time with whatever was passed in send_data.

That commit also sets math_needs_to_be_done back to .false. after done doing the math. Otherwise if you call diag_send_complete twice in a row with no new send_data calls in between the math is going to be done twice.

@rem1776 rem1776 merged commit d15c35a into NOAA-GFDL:main Feb 13, 2024
2 checks passed
uramirez8707 added a commit to uramirez8707/FMScoupler that referenced this pull request Apr 2, 2024
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