mom-ocean / MOM6

Modular Ocean Model
Other
184 stars 224 forks source link

Add buoyancy flux to applyBoundaryFluxes() #543

Open adcroft opened 7 years ago

adcroft commented 7 years ago

In issue NOAA-GFDL/MOM6-examples#128 it was noted by @StephenGriffies that multiple calls to extractFluxes1d() is wasteful (and in that issue was causing complications with diagnostics). A second call to extractFluxes1d() is being made in order to calculate a buoyancy flux. Note that this second call has different arguments/options hard-coded. We would be better served calling extractFluxes1d() just once from applyBoundaryFluxes() and calculating a buoyancy flux there.

breichl commented 7 years ago

I have been working on an alternative fix to 128 that is in line with what you describe here. I should be able to push it up tomorrow. I've got it working in a way that doesn't change answers for now, but will include comments in the code for some details we still need to think about.

On Sun, Jul 2, 2017 at 4:17 PM, Alistair Adcroft notifications@github.com wrote:

In issue NOAA-GFDL/MOM6-examples#128 https://github.com/NOAA-GFDL/MOM6-examples/issues/128 it was noted by @StephenGriffies https://github.com/stephengriffies that multiple calls to extractFluxes1d() is wasteful (and in that issue was causing complications with diagnostics). A second call to extractFluxes1d() is being made in order to calculate a buoyancy flux. Note that this second call has different arguments/options hard-coded. We would be better served calling extractFluxes1d() just once from applyBoundaryFluxes() and calculating a buoyancy flux there.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/NOAA-GFDL/MOM6/issues/543, or mute the thread https://github.com/notifications/unsubscribe-auth/AJQH2HCalupMnP3Hmvk8XsBrHRItHHm0ks5sJ_rBgaJpZM4OLrh8 .

adcroft commented 7 years ago

Looking forward to your cleaner fix. #544 does not remove need to clean this up. You might need to resolve some conflicts though.

-- Dr Alistair Adcroft (Alistair.Adcroft@noaa.gov) Princeton University Tel: (609) 987-5073 NOAA/GFDL, 201 Forrestal Road, Princeton, NJ 08540

On Sun, Jul 2, 2017 at 8:31 PM, Brandon Reichl notifications@github.com wrote:

I have been working on an alternative fix to 128 that is in line with what you describe here. I should be able to push it up tomorrow. I've got it working in a way that doesn't change answers for now, but will include comments in the code for some details we still need to think about.

On Sun, Jul 2, 2017 at 4:17 PM, Alistair Adcroft <notifications@github.com

wrote:

In issue NOAA-GFDL/MOM6-examples#128 https://github.com/NOAA-GFDL/MOM6-examples/issues/128 it was noted by @StephenGriffies https://github.com/stephengriffies that multiple calls to extractFluxes1d() is wasteful (and in that issue was causing complications with diagnostics). A second call to extractFluxes1d() is being made in order to calculate a buoyancy flux. Note that this second call has different arguments/options hard-coded. We would be better served calling extractFluxes1d() just once from applyBoundaryFluxes() and calculating a buoyancy flux there.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/NOAA-GFDL/MOM6/issues/543, or mute the thread https://github.com/notifications/unsubscribe-auth/ AJQH2HCalupMnP3Hmvk8XsBrHRItHHm0ks5sJ_rBgaJpZM4OLrh8 .

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/NOAA-GFDL/MOM6/issues/543#issuecomment-312526412, or mute the thread https://github.com/notifications/unsubscribe-auth/AFlo84cQip8FvFalMB10mwJ_R5CTl9Swks5sKDZIgaJpZM4OLrh8 .

breichl commented 7 years ago

Should I wait for you to merge 544 so I can update my changes for the conflicts?

BTW, my fix isn't the cleanest, but it does eliminate the second call to extractfluxes1d while taking some necessary steps to avoid changing answers. I'll push the code up once you let me know your preference, but FYI what I have done is:

1) Add additional (optional) outputs to extractfluxes1d of netheat, netsalt, netMassInOut, and pen_sw_bnd that are prescribed to use (a) dt=1 (required to avoid answer change due to round-off because the call in applyboundaryfluxesinout uses the actual value of dt) and (b) the hard-coded choices for using river/calving fluxes. Then I add a loop at the end of applyboundaryfluxesinout to compute the (now 2d) surface buoyancy flux array from those output rates. The loop in applyboundaryfluxesinout is only called if the optional output (the 2d surface buoyancy flux) is requested.

Thus, with the optional arguments present extractfluxes1d is only called once and answers are not changed. However, if we accept answer changes due to round-off, we do not need the additional dt=1 outputs of netsalt, netmassinout, and pen_sw_bnd; but we still need the additional netheat output to account for the difference in river/calving heat fluxes flags in the OM4_05 test case (the quarter degree has these set to false, which perhaps should be changed to true at somepoint).

On Mon, Jul 3, 2017 at 12:59 PM, Alistair Adcroft notifications@github.com wrote:

Looking forward to your cleaner fix. #544 does not remove need to clean this up. You might need to resolve some conflicts though.

-- Dr Alistair Adcroft (Alistair.Adcroft@noaa.gov) Princeton University Tel: (609) 987-5073 NOAA/GFDL, 201 Forrestal Road, Princeton, NJ 08540

On Sun, Jul 2, 2017 at 8:31 PM, Brandon Reichl notifications@github.com wrote:

I have been working on an alternative fix to 128 that is in line with what you describe here. I should be able to push it up tomorrow. I've got it working in a way that doesn't change answers for now, but will include comments in the code for some details we still need to think about.

On Sun, Jul 2, 2017 at 4:17 PM, Alistair Adcroft < notifications@github.com

wrote:

In issue NOAA-GFDL/MOM6-examples#128 https://github.com/NOAA-GFDL/MOM6-examples/issues/128 it was noted by @StephenGriffies https://github.com/stephengriffies that multiple calls to extractFluxes1d() is wasteful (and in that issue was causing complications with diagnostics). A second call to extractFluxes1d() is being made in order to calculate a buoyancy flux. Note that this second call has different arguments/options hard-coded. We would be better served calling extractFluxes1d() just once from applyBoundaryFluxes() and calculating a buoyancy flux there.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/NOAA-GFDL/MOM6/issues/543, or mute the thread https://github.com/notifications/unsubscribe-auth/ AJQH2HCalupMnP3Hmvk8XsBrHRItHHm0ks5sJ_rBgaJpZM4OLrh8 .

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/NOAA-GFDL/MOM6/issues/543#issuecomment-312526412, or mute the thread https://github.com/notifications/unsubscribe- auth/AFlo84cQip8FvFalMB10mwJ_R5CTl9Swks5sKDZIgaJpZM4OLrh8 .

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/NOAA-GFDL/MOM6/issues/543#issuecomment-312694353, or mute the thread https://github.com/notifications/unsubscribe-auth/AJQH2MsxWeGz_qdQOrLwIxTMKhnXM-nqks5sKR35gaJpZM4OLrh8 .

adcroft commented 7 years ago

It would be simplest to wait for the merges. We're all waiting for Bob to click the "Merge Pull Request" button on this and other requests.

Is it truly round-off difference for OM4_05? You'll have to ask John D. whether he'll accept that.

I agree about turning on river heat fluxes. I need to put a run in with this.

-- Dr Alistair Adcroft (Alistair.Adcroft@noaa.gov) Princeton University Tel: (609) 987-5073 NOAA/GFDL, 201 Forrestal Road, Princeton, NJ 08540

On Mon, Jul 3, 2017 at 1:46 PM, Brandon Reichl notifications@github.com wrote:

Should I wait for you to merge 544 so I can update my changes for the conflicts?

BTW, my fix isn't the cleanest, but it does eliminate the second call to extractfluxes1d while taking some necessary steps to avoid changing answers. I'll push the code up once you let me know your preference, but FYI what I have done is:

1) Add additional (optional) outputs to extractfluxes1d of netheat, netsalt, netMassInOut, and pen_sw_bnd that are prescribed to use (a) dt=1 (required to avoid answer change due to round-off because the call in applyboundaryfluxesinout uses the actual value of dt) and (b) the hard-coded choices for using river/calving fluxes. Then I add a loop at the end of applyboundaryfluxesinout to compute the (now 2d) surface buoyancy flux array from those output rates. The loop in applyboundaryfluxesinout is only called if the optional output (the 2d surface buoyancy flux) is requested.

Thus, with the optional arguments present extractfluxes1d is only called once and answers are not changed. However, if we accept answer changes due to round-off, we do not need the additional dt=1 outputs of netsalt, netmassinout, and pen_sw_bnd; but we still need the additional netheat output to account for the difference in river/calving heat fluxes flags in the OM4_05 test case (the quarter degree has these set to false, which perhaps should be changed to true at somepoint).

On Mon, Jul 3, 2017 at 12:59 PM, Alistair Adcroft < notifications@github.com> wrote:

Looking forward to your cleaner fix. #544 does not remove need to clean this up. You might need to resolve some conflicts though.

-- Dr Alistair Adcroft (Alistair.Adcroft@noaa.gov) Princeton University Tel: (609) 987-5073 NOAA/GFDL, 201 Forrestal Road, Princeton, NJ 08540

On Sun, Jul 2, 2017 at 8:31 PM, Brandon Reichl <notifications@github.com

wrote:

I have been working on an alternative fix to 128 that is in line with what you describe here. I should be able to push it up tomorrow. I've got it working in a way that doesn't change answers for now, but will include comments in the code for some details we still need to think about.

On Sun, Jul 2, 2017 at 4:17 PM, Alistair Adcroft < notifications@github.com

wrote:

In issue NOAA-GFDL/MOM6-examples#128 https://github.com/NOAA-GFDL/MOM6-examples/issues/128 it was noted by @StephenGriffies https://github.com/stephengriffies that multiple calls to extractFluxes1d() is wasteful (and in that issue was causing complications with diagnostics). A second call to extractFluxes1d() is being made in order to calculate a buoyancy flux. Note that this second call has different arguments/options hard-coded. We would be better served calling extractFluxes1d() just once from applyBoundaryFluxes() and calculating a buoyancy flux there.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/NOAA-GFDL/MOM6/issues/543, or mute the thread https://github.com/notifications/unsubscribe-auth/ AJQH2HCalupMnP3Hmvk8XsBrHRItHHm0ks5sJ_rBgaJpZM4OLrh8 .

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/NOAA-GFDL/MOM6/issues/543#issuecomment-312526412, or mute the thread https://github.com/notifications/unsubscribe- auth/AFlo84cQip8FvFalMB10mwJ_R5CTl9Swks5sKDZIgaJpZM4OLrh8 .

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/NOAA-GFDL/MOM6/issues/543#issuecomment-312694353, or mute the thread https://github.com/notifications/unsubscribe-auth/AJQH2MsxWeGz_ qdQOrLwIxTMKhnXM-nqks5sKR35gaJpZM4OLrh8 .

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/NOAA-GFDL/MOM6/issues/543#issuecomment-312702657, or mute the thread https://github.com/notifications/unsubscribe-auth/AFlo86SZ2R3We-dZD5dbNY9NdXf3LzZeks5sKSjogaJpZM4OLrh8 .

breichl commented 7 years ago

No problem then, I'll hold off.

Is it truly round-off difference for OM4_05? You'll have to ask John D. whether he'll accept that.

Yes, several of the additional outputs are only required by round-off. I could alternatively take the netsalt, netmassinout, and pen_SW_bnd ouput from extractfluxes1d and divide by the DT value it is called with. But, unfortunately N*3600./3600. does not quite equal N. I talked to Bob and he was willing to accept this, but since I have to add an extra netheat output regardless I just included all 4 outputs for now (to satisfy the previous answers).

On Mon, Jul 3, 2017 at 1:56 PM, Alistair Adcroft notifications@github.com wrote:

It would be simplest to wait for the merges. We're all waiting for Bob to click the "Merge Pull Request" button on this and other requests.

Is it truly round-off difference for OM4_05? You'll have to ask John D. whether he'll accept that.

I agree about turning on river heat fluxes. I need to put a run in with this.

-- Dr Alistair Adcroft (Alistair.Adcroft@noaa.gov) Princeton University Tel: (609) 987-5073 NOAA/GFDL, 201 Forrestal Road, Princeton, NJ 08540

On Mon, Jul 3, 2017 at 1:46 PM, Brandon Reichl notifications@github.com

wrote:

Should I wait for you to merge 544 so I can update my changes for the conflicts?

BTW, my fix isn't the cleanest, but it does eliminate the second call to extractfluxes1d while taking some necessary steps to avoid changing answers. I'll push the code up once you let me know your preference, but FYI what I have done is:

1) Add additional (optional) outputs to extractfluxes1d of netheat, netsalt, netMassInOut, and pen_sw_bnd that are prescribed to use (a) dt=1 (required to avoid answer change due to round-off because the call in applyboundaryfluxesinout uses the actual value of dt) and (b) the hard-coded choices for using river/calving fluxes. Then I add a loop at the end of applyboundaryfluxesinout to compute the (now 2d) surface buoyancy flux array from those output rates. The loop in applyboundaryfluxesinout is only called if the optional output (the 2d surface buoyancy flux) is requested.

Thus, with the optional arguments present extractfluxes1d is only called once and answers are not changed. However, if we accept answer changes due to round-off, we do not need the additional dt=1 outputs of netsalt, netmassinout, and pen_sw_bnd; but we still need the additional netheat output to account for the difference in river/calving heat fluxes flags in the OM4_05 test case (the quarter degree has these set to false, which perhaps should be changed to true at somepoint).

On Mon, Jul 3, 2017 at 12:59 PM, Alistair Adcroft < notifications@github.com> wrote:

Looking forward to your cleaner fix. #544 does not remove need to clean this up. You might need to resolve some conflicts though.

-- Dr Alistair Adcroft (Alistair.Adcroft@noaa.gov) Princeton University Tel: (609) 987-5073 NOAA/GFDL, 201 Forrestal Road, Princeton, NJ 08540

On Sun, Jul 2, 2017 at 8:31 PM, Brandon Reichl < notifications@github.com

wrote:

I have been working on an alternative fix to 128 that is in line with what you describe here. I should be able to push it up tomorrow. I've got it working in a way that doesn't change answers for now, but will include comments in the code for some details we still need to think about.

On Sun, Jul 2, 2017 at 4:17 PM, Alistair Adcroft < notifications@github.com

wrote:

In issue NOAA-GFDL/MOM6-examples#128 https://github.com/NOAA-GFDL/MOM6-examples/issues/128 it was noted by @StephenGriffies https://github.com/stephengriffies that multiple calls to extractFluxes1d() is wasteful (and in that issue was causing complications with diagnostics). A second call to extractFluxes1d() is being made in order to calculate a buoyancy flux. Note that this second call has different arguments/options hard-coded. We would be better served calling extractFluxes1d() just once from applyBoundaryFluxes() and calculating a buoyancy flux there.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/NOAA-GFDL/MOM6/issues/543, or mute the thread https://github.com/notifications/unsubscribe-auth/ AJQH2HCalupMnP3Hmvk8XsBrHRItHHm0ks5sJ_rBgaJpZM4OLrh8 .

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <https://github.com/NOAA-GFDL/MOM6/issues/543#issuecomment-312526412 , or mute the thread https://github.com/notifications/unsubscribe- auth/AFlo84cQip8FvFalMB10mwJ_R5CTl9Swks5sKDZIgaJpZM4OLrh8 .

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/NOAA-GFDL/MOM6/issues/543#issuecomment-312694353, or mute the thread https://github.com/notifications/unsubscribe-auth/AJQH2MsxWeGz_ qdQOrLwIxTMKhnXM-nqks5sKR35gaJpZM4OLrh8 .

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/NOAA-GFDL/MOM6/issues/543#issuecomment-312702657, or mute the thread https://github.com/notifications/unsubscribe-auth/AFlo86SZ2R3We- dZD5dbNY9NdXf3LzZeks5sKSjogaJpZM4OLrh8 .

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/NOAA-GFDL/MOM6/issues/543#issuecomment-312704485, or mute the thread https://github.com/notifications/unsubscribe-auth/AJQH2JySmLQw3_5Bkk3srYzhUZ7M_zdIks5sKStqgaJpZM4OLrh8 .

StephenGriffies commented 7 years ago

When I return after a brief holiday, I will try to check what you already ran, Brandon, to see if the hfds diagnostic is correct based on your changes.

StephenGriffies commented 4 years ago

@breichl any reason to keep this issue open? Can you please check?

breichl commented 4 years ago

I left some notes about this in the code and a commit, which concluded with "This commit does not change answers, but similar to #544, this is not a permanent fix as the code could be simplified to remove '_rate' terms (will introduce round-off answer changes) and a more permanent solution for useRiverHeatContent and useCalvingHeatContent. These points are described within code comments."

We have a protocol established now of adding flags with the new code that removes the round-off error associated with the duplicate call (to get the '_rate' terms), and then changing answers and deleting old code. So I think this should probably be kept open for now, and I'll add the flags to clean that part up. I'm unsure what the issues were with useRiverHeatContent and useCalvingHeatContent, so will revisit this when I'm looking at the code again.

StephenGriffies commented 4 years ago

Thanks @breichl