NorESMhub / BLOM

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

science support attributes for compsets in BLOM for v1.5.0 #320

Closed mvertens closed 6 months ago

mvertens commented 7 months ago

It looks like there is still the science_support attribute set in the blom config_compsets.xml. I think this attribute is not being used correctly in BLOM. First - if that is the case - there should always be a test associated with that. Second - there should be released simulations for the science supported configurations. Do we still want to keep this for the noresm2.1 release?

mvertens commented 7 months ago

@TomasTorsvik @matsbn @JorgSchwinger - I'm happy to create a PR removing the science support attributes if you agree with my comments.

JorgSchwinger commented 7 months ago

Thank you @mvertens for bringing this up. I have two questions

1) There should be a test, but in reference to what? To an older case, or a test using this compset a s a baseline? 2) There should be released simulations - isn't this very restrictive? This means we can only "scientifically support" simulations that are either available on ESGF or that have been published? elsewhere with a doi?

For BLOM/iHAMOCC stand-alone compsets the release 2.1.0 seems to be very similar to 2.0.6, I have run 600 years of NOINYOC_T62_tn21 and it is practically identical (which is good, since it indicates that the bug-fixes have only minor impacts)

mvertens commented 7 months ago

@JorgSchwinger - thanks so much for your quick response. Sorry for not being clear.

  1. By a test I mean a regression test in blom/cime_config/testdefs/testlist_blom.xml. CIME checks if there is a corresponding test for the compset/resolution pair and if so you do not have to add a run-unsupported to your create_newcase command.
  2. This is an excellent question and we should define it across the system. In CESM we linked the diagnostics of the scientifically supported simulations from the release web page.

If you feel that NOINYOC_T62_tn21 is scientifically supported in release2.1, then we need to decide where we display the results. Then I think we should add a test for that and make sure it works out of the box on betzy. This might be adding to many requirements to this release - but I it would be an excellent start for the upcoming noresm2.3 release in the late winter.

Due to the difficulties of providing full scientific documentation for changes introduced by bug fixes in this release (even potentially minor ones) it was decided that NorESM2.1 would be a technical release. That means that no compsets across the system are required to be scientifically supported. For example CAM is removing all science supported attributes on compsets.

TomasTorsvik commented 6 months ago

I think we can remove the science_support attribute for noresm2.1, but it's good to discuss the procedure for upcoming releases. As long as we are in the INES2-interim period, it is most natural to use the NS9560K project. However, I don't know what the long term plans are for NS9560K (or NS2345K) going forward.

mvertens commented 6 months ago

@TomasTorsvik - thanks that makes sense to me. We need to create a release branch to do this. I would also like to remove blom regressions tests that can only be run out of the noresm2.5 development code base (i.e. that require ww3dev). Could you please create the release branch - I'm not sure what the best name would be. How should we tag it?

TomasTorsvik commented 6 months ago

@mvertens - I have created a release branch release-1.5. I suggest we tag commits on this branch with v1.5.1, v1.5.2, etc.

mvertens commented 6 months ago

@TomasTorsvik - thanks so much for this. I think there is a problem with using the tags v1.5.1 - since I will be removing regression tests that only work with checkouts where ww3dev is done (this is not the case in noresm2.1 or noresm2.3 but is the case with noresm2.5). I suggest that we tag this as release_1.5.1, release_1.5.2, etc. Does this make sense?

TomasTorsvik commented 6 months ago

@mvertens - I don't quite understand the problem. If there is something in v1.5.0 that we want to remove that only concerns NorESM builds and not BLOM/iHAMOCC source code, and then make a new BLOM/iHAMOCC release, I would think the logical tag name should be v1.5.1. But maybe I'm not quite on board with what you want to achieve, or maybe we have different ideas what the v1.5.# tags should represent?

gold2718 commented 6 months ago

I think the confusion might be having tag names match branch names.

My understanding is that v1.5.0 is tagging a commit on the master branch (takes a bit of work since it does not seem to be an annotated tag). That makes me think that tag names such as v1.5.1 etc. should be for tags on the master branch.

Perhaps we can use a different naming scheme for tags on the release branch (release-1.5)?

TomasTorsvik commented 6 months ago

@gold2718 , @mvertens - We had a discussion about the BLOM/iHAMOCC tags revcently, in #295 . Specifically, v#.#.# tags should correspond to code releases, and dev#.#.# should follow master (see https://github.com/NorESMhub/BLOM/discussions/295#discussioncomment-7652719). What maybe caused confusion was that I created v1.5.0 on master because there was no need to create a separate release branch at that point. Having release branches named release-#.# with release tags v#.#.# follows what we have done previously for BLOM/iHAMOCC.

We are planning to introduce automated dev#.#.# tags on master from now on, but this has not been introduced yet.

gold2718 commented 6 months ago

@TomasTorsvik Aha! So because the release-1.5 branch was created from master after the v1.5 tag was made, they will point to the same release anyway. I (now) think your proposal is good, thanks for the patience in explaining it.

mvertens commented 6 months ago

@TomasTorsvik - sorry the confusion is totally on my end. You are totally right.

TomasTorsvik commented 6 months ago

I have made a new tag v1.5.1 on the branch release-1.5, where the science_support attribute has been removed.