search

NorESMhub / BLOM

Bergen Layered Ocean Model
GNU Lesser General Public License v3.0
16 stars 25 forks source link

Remove dmspar variable #300

Closed JorgSchwinger closed 8 months ago

JorgSchwinger commented 8 months ago

The dmspar variable was used to define the parameters of the DMS scheme, but it was copied over to variables dmsp1-dmsp6. This PR simplifies this by directly defining dsmp1-dsmp6 in mo_param_bgc.

jmaerz commented 8 months ago

Hi @JorgSchwinger , I just realized that you could outsource the multiplication with dtb from ocprod to mo_param_bgc as well... - but up to you.

mvertens commented 8 months ago

@JorgSchwinger @jmaerz @TomasTorsvik - since we are migrating this code to master - it would be good moving forwards to run the regression test suite and determine if answers change by comparing to baselines and generating baselines. Are only stand-alone tests being run for these updates - or are tests using blom within NorESM being used? And it might be good to add new compsets that might trigger new added features. Should I try to arrange a meeting next week to talk about this?

JorgSchwinger commented 8 months ago

Hi @JorgSchwinger , I just realized that you could outsource the multiplication with dtb from ocprod to mo_param_bgc as well... - but up to you.

Yes, this is not nice, but note that this was no change in comparison to before. Not sure whether to change this now.

JorgSchwinger commented 8 months ago

I have tested this in stand-alone getting bit-for-bit identical results. We will need new baselines when we have finished v1.4.0 and move towards v1.5.0, I guess. I'm happy to meet next week.

jmaerz commented 8 months ago

As written, up to you - I haven't really had a look into DMS before, but I realized that these are rates, which one could/would want to tune with changing physics, etc. - and just to be in line with what we have thus far (converting rates per s or d to per time step). From my side, just go ahead and merge. I just had the impression that we aimed at a cleaner code in general for this release.

From: Jörg Schwinger @.***> Sent: Friday, November 3, 2023 2:58 PM To: NorESMhub/BLOM Cc: Joeran Maerz; Mention Subject: Re: [NorESMhub/BLOM] Remove dmspar variable (PR #300)

Hi @JorgSchwingerhttps://github.com/JorgSchwinger , I just realized that you could outsource the multiplication with dtb from ocprod to mo_param_bgc as well... - but up to you.

Yes, this is not nice, but note that this was no change in comparison to before. Not sure whether to change this now.

— Reply to this email directly, view it on GitHubhttps://github.com/NorESMhub/BLOM/pull/300#issuecomment-1792487571, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AWAIMLRODKLP4MFDVRN722TYCT2BHAVCNFSM6AAAAAA64KT2Y6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOJSGQ4DONJXGE. You are receiving this because you were mentioned.Message ID: @.***>

JorgSchwinger commented 8 months ago

I might try to fix the dtb issue in a separate PR (it's the same issue for the BrO scheme). Was a bit worried about bfb.

jmaerz commented 8 months ago

@JorgSchwinger , all fine - understandable ( I was also crossing fingers when moving all the parameters around). Wrt meeting next week, @mvertens , I would be also fine with it.