Updates to support two versions of Catchment-CN: Catchment-CN.4.0 and Catchment-CN.4.5

This PR is in response to issue #41 . The objective is to support two Catchment-CN versions, Catchment-CN.4.0 and Catchment-CN.4.5, and to allow the user to toggle between them.
To support both versions, the GEOScatchCN_GridComp directory has two subdirectories 'CLM40' and 'CLM45'. To toggle between both CatchCN versions it is currently necessary manually change the CMakeLists.txt files in the GEOScatchCN_GridComp and GEOSland_GridComp directories to point at the desired CLM subdirectory. If possible, it may make sense to implement this toggle at a higher level and in one place in the future.
Because CLM4.0 and CLM4.5 are different model versions, there are extensive differences in the CN routines (in the respective CLM subdirectory), which I will not list here.
Restart files differ between both model versions (affects NUM_PFT, NUM_COL, CNPFT, CNCOL, some fixed parameters, and running means).
CLM4.5 has additional diagnostics. Modifications have been made to GEOS_SurfaceGridComp.F90 and GEOS_LandGridComp.F90 to support these.
Additional files affected by this:
Scale_CatchCN.F90:

added ability to handle Catchment-CN.4.5 files
fixed bug that got in during MAPL2 upgrade and that resulted in array CNPFT being read with incorrect dimensions, leading to multiple carbon variables being set to 0 (this bug affected both CNCLM40 and CNCLM45)

lsm_routines.F90:

added optional surface wetness outputs (swsrf1out, swsrf2out, swsrf4out) to catch_calc_soil_moist subroutine that are used in CLM4.5 simulations

SurfParams.F90:

added new collection 'CN_CLM45' to be used with Catchment-CN.4.5; this parameter set reproduces LDASsa Catchment-CN.4.5 simulations generated by E.Lee for M. Folette-Cook's MAP project.

GEOS_SurfaceGridComp.rc:

added documentation for 'CN_CLM45' parameter set

NOTE: The implementation of both Catchment-CN versions will require updates to GEOSldas and GEOSgcm_App repositories and will be linked to pull requests in both.
GEOSldas PR 326
GEOSgcm_App PR 177
CAVEAT: In its current version, this branch runs fine in offline mode and has been subject to some science-validation. Additional modifications are required to be able to run Catchment-CN.4.5 with the AGCM. These are currently implemented by @smahanam .
cc: @gmao-rreichle , @biljanaorescanin , @rdkoster , @smahanam , @weiyuan-jiang

I am going to ping @tclune on this PR as the CMake here is sounds interesting/challenging.
For your current development, a possible temporary solution is to use CMake option(). CMake has the ability to do build-time options, so you could have:
cmake .. -DBASEDIR=$BASEDIR/Linux -DCMAKE_INSTALL_PREFIX=../install -DCATCHCN_VERSION=4.0 ...

Or whatever you like. That CATCHCN_VERSION would then trigger which one you want to build. This could at least make your testing/development right now a bit easier.
However, if you ever anticipate wanting to have both codes built and and available to users, we might have to look at what Tom did for MOM5/MOM6 which was to use shared object libraries that are loaded at runtime. Then you could choose when you run the experiment do you load CN 4.0 or CN 4.5. This requires some careful CMake and maybe some renames here and there.

I concur with what @mathomp4 wrote.       This might be better than the MOM5+MOM6 situation where many intermediate interfaces remained the same which trips up the loader.  (execution follows the correct component but then calls procedures in the other version.)
I encourage pursuing the "build both" option.

I agree with @tclune that "build both" would be best.  Failing that, we would need to implement the suggestion by @mathomp4 that lets the user choose the model version to be built with a simple compile flag.  Manual editing of CMake files by users is not workable.  (How would we implement this in the nightly tests?)
Keep in mind that CatchCN-CLM4.5 is only a stepping stone towards yet another version, CatchCN-CLM5.  Chances are that at any given time more than one versions will be used for science.
PS: I fixed the broken URL links in the comments in this PR and the related GEOSgcm_app and GEOSldas PRs.

Thank you all for your feedback on this! I will start implementing a version that builds both models. Given my relative inexperience with CMake, I may need some help with this step.
Another question that I have (probably for Rolf), at what point would we want the user to chose which model they run? Should that be controlled through ldas_setup (and the AGCM equivalent) or is there a better place for this?

I am not sure how build both works. It sounds like 2 separate libraries, then we probably need a separate set of connectivity. etc
From my experience on CATCHCN applications and interactions with people that run CATCHCN, @mathomp4 's suggestion is nice and neat. Ideally, if we could have a default version with a build-time option to override the default version with any other desired version would be great.
Matt, please we would count you to make this change to CMakeLists.txt files. It was you who provided me this version many months ago for us to implement multiple versions of the CN model. Please check in your changes, I will test it at my end.

at what point would we want the user to chose which model they run? Should that be controlled through ldas_setup (and the AGCM equivalent)
Can this be done in the rc file?  I suppose LSM_CHOICE and/or LSM_PARAMS could be used to select the specific version of CatchCN.

All, as @tclune said, that "build both at the same time" for MOM5 and MOM6 had issues because both models defined, say, ocean_init() and so run problems were encountered: "Which ocean_init() is called?" or worse which "do_ocean_calc()" is caled...
Now, one way to fix it would have been to rename all the common bits in one model as ocean_init_mom5() or the like. Tedious, but doable. But Tom figured out a clever way to use DSOs and LD_PRELOAD to make it work (though, as I've learned recently, maybe not quite completely yet). This was a good choice here because both mom5 and mom6 are external codes that we really wanted to avoid having custom versions of.
But, for Catchment? It's possible that with some changes to gridcomps, etc. this can be done much easier. We have (or had) multiple dynamics, different radiations, etc. All are controlled through AGCM.rc with things like USE_RRTMG_IRRAD: 1.0 (though that's a bad switch. I was...not good at GEOS in those days).
Now the first suggestion (an option() at build time), is pretty simple. The build both takes effort, but it's worth it in the end!
A question for @gmao-jkolassa: To build this new code, is it self-contained in this branch? Or does it need changes in GMAO_Shared, etc.

To build this new code, is it self-contained in this branch? Or does it need changes in GMAO_Shared, etc.

@mathomp4: For building, this code is self-contained in this branch, i.e., there are no changes to GMAO_Shared. All the files that do need to be changed are the ones listed in bold in my original comment. Now, to run an actual experiment, additional changes have to be made to GEOSldas and GEOSgcm_App.

https://github.com/GEOS-ESM/GEOSgcm_GridComp/blob/4c39ea2d8094d572b16a9905701f23a2e8c1d787/GEOSagcm_GridComp/GEOSphysics_GridComp/GEOSsurface_GridComp/GEOSland_GridComp/GEOScatchCN_GridComp/CMakeLists.txt#L5-L8
Are you going to switch clm4.0 and 4.5 by editing the Cmakefile? @gmao-jkolassa . Why not make GEOScatchCN a component and make clm4.0 and  clm5.0 exclusive children?

Are you going to switch clm4.0 and 4.5 by editing the Cmakefile?

@weiyuan-jiang, that is how it is implemented right now, but I think the plan is to move away from having to manually edit the CMakeLists.txt.

Why not make GEOScatchCN a component and make clm4.0 and clm5.0 exclusive children?

Could you show me how to do that?

@gmao-jkolassa OK, let me think about it before final pull request

Well, I have a CMake edit that would allow a build-time switch: -DCATCHCN_VERSION=(4.0|4.5) that defaults to 4.0. I could commit it (here or on a branch) or I can wait for @weiyuan-jiang to look at the (better) children-of-CN.

@gmao-jkolassa @mathomp4 Look at this branch feature/jkolassa_cnclm45_jiang . I realize there are a lot files that have the same names which may create trouble if building simultaneously.  So we can have build-time switch. By default, it is clm40, later on you can change the default by switching
option(CLM4.0 "running clm4.0" OFF)
option(CLM4.5 "running clm4.5" ON)

@gmao-jkolassa @mathomp4 Look at this branch feature/jkolassa_cnclm45_jiang . I realize there are a lot files that have the same names which may create trouble if building simultaneously. So we can have build-time switch. By default, it is clm40, later on you can change the default by switching
option(CLM4.0 "running clm4.0" OFF)
option(CLM4.5 "running clm4.5" ON)

@weiyuan-jiang I did it a bit differently as I forgot option() was only boolean. So I added a variable to be set at build-time:
https://github.com/GEOS-ESM/GEOSgcm_GridComp/compare/feature/jkolassa_cnclm45...feature/mathomp4/selectcn
The set() is here:
https://github.com/GEOS-ESM/GEOSgcm_GridComp/blob/b8d662bde265c1e1811b6fcecd4fcd1eff3a4140/GEOSagcm_GridComp/GEOSphysics_GridComp/GEOSsurface_GridComp/GEOSland_GridComp/CMakeLists.txt#L14-L24
I then use that ${CATCHCN_COMPONENT} in the two CMakeLists.txt

I think we need to restructure a little bit. But your approach is neat. I will take advantage of that.

I adopt your tricks. @mathomp4 .  @gmao-jkolassa , please take a look at my branch and feel free to make your changes.

Thank you @weiyuan-jiang ! I will be taking a look at your branch today and test it for Catchment-CN.4.0 and Catchment-CN.4.5. If I understand it correctly, you implemented the build-time switch now. I think it would be good to nevertheless keep thinking about the run-time switch and what that would entail.

I have merged your changes to my branch feature/jkolassa_cnclm45_jiang. We can have runtime switch for sure .

Ok, thanks! The changes from this morning were just to bring my branch up to date with the latest develop.

Successfully tested building both Catchment-CN.4.0 and Catchment-CN.4.5. Merged @weiyuan-jiang 's branch into this one.

@gmao-jkolassa :
Issue #232 is the old CatchCN clean-up discussion that I had mentioned yesterday.
Contrary to what I had thought yesterday, #232 does not mention math_routines.F90 explicitly.
But there are still a couple of suggestions in #232 that are worth looking into as part of this pull request.
@weiyuan-jiang @biljanaorescanin @smahanam

@gmao-rreichle :
Thank you, Rolf! I sent my assessment of the CN files to @weiyuan-jiang and he is currently working on cleaning up the code according to what we discussed yesterday. Maybe this is good time for him to include your suggestions from issue #232 as well.

@gmao-jkolassa @rdkoster : I just realized that the new directory names and the language on the slides for the code review with Bill do not quite match what I had expected.
Instead of
I thought we would have something like:
./GEOScatchCNCLM40_GridComp
./GEOScatchCNCLM45_GridComp
This is a bit subtle, but the "40" and "45" numbering goes with "CLM", not "CN", unless we lock in the CLM versioning for CN.
If we ever have an update to the hydrology without changing the underlying CLM version, we could then use something like
"CN2CLM4x".
Maybe rename the directories accordingly?
cc: @weiyuan-jiang @biljanaorescanin

Rolf,
that is a good point. I think your suggested naming scheme makes sense.

Is the LSM_CHOICE redundant in the SurfParams.F90 ? @gmao-jkolassa

Is the LSM_CHOICE redundant in the SurfParams.F90 ? @gmao-jkolassa

It shouldn't be. We still need the LSM_CHOICE check since certain LAND_PARAMS collections should only be used with Catchment, while others should only be used with Catchment-CN. Also, the parameter sets to reproduce the science-validated runs of Catchment-CN4.0 and Catchment-CN4.5 are slightly different. So I would suggest to keep LSM_CHOICE is SurfParams.F90 for now. @weiyuan-jiang

When I run clm45,  the debugging error message clearly shows that the compiler links to the wrong subroutine CN_DriverMod.F90 which exists in CNCLM40 and CNCLM45,  i.e.,  GEOS_CatchCNCLM45GridComp links to CLM40's CN_DriverMod.F90. So we need DSO strategy. @gmao-jkolassa @tclune @gmao-rreichle
forrtl: severe (408): fort: (2): Subscript #3 of the array CNCOL has value 36 which is greater than the upper bound of 35
Image              PC                Routine            Line        Source
GEOSldas.x<PHONE_NUMBER>F7976F  Unknown               Unknown  Unknown
GEOSldas.x<PHONE_NUMBER>1E3691  cn_drivermod_mp_c         823  CN_DriverMod.F90
GEOSldas.x<PHONE_NUMBER>813770  geos_catchcnclm45        4169  GEOS_CatchCNCLM45GridComp.F90

I eventually figure out why it crashes. Basically the linking is a mess and the final GEOSldas.x is not what we want.  If we build cnclm40 and cnclm45 as dynamic shared libraries, they are running fine.  But to do so, we need support from new MAPL

This PR is contingent on GEOSgcm PR 246, which updates MAPL to v2.5.0.

This PR is contingent on GEOSgcm PR 246, which updates MAPL to v2.5.0.

@sdrabenh this is ready for review. I did a simple test and it was zero diff for GCM. You will need MAPL 2.5 when running tests.

https://github.com/GEOS-ESM/GEOSgcm_App/pull/177

@sdrabenh this is ready for review. I did a simple test and it was zero diff for GCM. You will need MAPL 2.5 when running tests.

https://github.com/GEOS-ESM/GEOSgcm_App/pull/177

Unless @weiyuan-jiang or someone has updated the below program (+ 2 perl scripts):
https://github.com/GEOS-ESM/GEOSgcm_GridComp/blob/develop/GEOSagcm_GridComp/GEOSphysics_GridComp/GEOSsurface_GridComp/GEOSland_GridComp/GEOScatch_GridComp/mk_restarts/mk_CatchCNRestarts.F90
technically, this is not ready. Maybe, I am missing something.

Unless @weiyuan-jiang or someone has updated the below program (+ 2 perl scripts):
https://github.com/GEOS-ESM/GEOSgcm_GridComp/blob/develop/GEOSagcm_GridComp/GEOSphysics_GridComp/GEOSsurface_GridComp/GEOSland_GridComp/GEOScatch_GridComp/mk_restarts/mk_CatchCNRestarts.F90
technically, this is not ready. Maybe, I am missing something.

I'm surprised by @smahanam 's comment that mk_CatchCNRestarts.F90 may not be ready.
Since @gmao-jkolassa  was able to run both CNCLM4.0 and CNCLM4.5, we should at least have some restarts that work.
If that's the case, then we should probably proceed with this set of PRs and sort out any remaining issues with mk_CatchCNRestarts.F90 later.
Or are we missing something more fundamental?
cc: @biljanaorescanin @weiyuan-jiang @rdkoster @sdrabenh

I'm surprised by @smahanam 's comment that mk_CatchCNRestarts.F90 may not be ready.
Since @gmao-jkolassa  was able to run both CNCLM4.0 and CNCLM4.5, we should at least have some restarts that work.
If that's the case, then we should probably proceed with this set of PRs and sort out any remaining issues with mk_CatchCNRestarts.F90 later.
Or are we missing something more fundamental?
cc: @biljanaorescanin @weiyuan-jiang @rdkoster @sdrabenh

I am surprised too. I double-checked the restart files that were written for the CNCLM4.5 simulations using this branch and they appear correct, i.e. arrays in the restart files have dimensions corresponding to CNCLM4.5 and not CNCLM4.0.
I agree with @smahanam that this needs to be addressed, but possibly this could be done independently of this PR as suggested by @gmao-rreichle .

I am surprised too. I double-checked the restart files that were written for the CNCLM4.5 simulations using this branch and they appear correct, i.e. arrays in the restart files have dimensions corresponding to CNCLM4.5 and not CNCLM4.0.
I agree with @smahanam that this needs to be addressed, but possibly this could be done independently of this PR as suggested by @gmao-rreichle .

I also don't know, I wasn't following this since I handed over the branch to Jana few months ago.
Jana, the program that I mentioned and the 2 perl scripts are mk_GEOSldasRestarts.F90/ldas_setup/preprocess_rst.csh equivalent for the AGCM. You and I communicate frequently, I am not sure you ever ran AGCM (Maybe, you did, then I withdraw my comment)
Unless, Weiyuan or someone has updated those AGCM side files (I did not do it when I first created this experimental branch for Melanie's project), this PR breaks AGCM (CATCHCN applications).

I also don't know, I wasn't following this since I handed over the branch to Jana few months ago.
Jana, the program that I mentioned and the 2 perl scripts are mk_GEOSldasRestarts.F90/ldas_setup/preprocess_rst.csh equivalent for the AGCM. You and I communicate frequently, I am not sure you ever ran AGCM (Maybe, you did, then I withdraw my comment)
Unless, Weiyuan or someone has updated those AGCM side files (I did not do it when I first created this experimental branch for Melanie's project), this PR breaks AGCM (CATCHCN applications).

@smahanam We have only tested this branch for offline simulations so far. Since the GCM currently does not use Catchment-CN, the changes subject to this PR should not have any impact (and I believe @biljanaorescanin has verified this). I believe that a few weeks back we agreed that the AGCM compatibility of CNCLM45 that will be required for Melanie's project would be implemented after we have merged this branch into 'develop'. Maybe I misunderstood?

@smahanam We have only tested this branch for offline simulations so far. Since the GCM currently does not use Catchment-CN, the changes subject to this PR should not have any impact (and I believe @biljanaorescanin has verified this). I believe that a few weeks back we agreed that the AGCM compatibility of CNCLM45 that will be required for Melanie's project would be implemented after we have merged this branch into 'develop'. Maybe I misunderstood?

Apologies for the obvious breakdown in communication.  Let me try to summarize today's discussion so far (based on the above and offline emails).  If I understand things correctly, merging the PR at this moment would:

facilitate GEOSldas (land-only) simulations with CNCLM4.0 and CNCLM4.5;
temporarily break the current capability to run the AGCM with CNCLM4.0 (Fanwei's science-tested version);
be only a stepping stone towards supporting Melanie's fire work (AGCM with CNCLM4.5).

To address 2. and 3., more work is needed per @smahanam 's comments (CatchCNRestarts.F90 and 2 perl scripts (preprocess_rst.csh and equivalent AGCM script).  Unless a higher priority is assigned to this, @smahanam would not immediately be available to help with this.
The question, mostly for @gmao-jkolassa, @rdkoster and @wmputman, is whether we should move ahead with the PR at this time.  The answer depends on whether there's a need for 1. and whether 2. and 3. are acceptable at this time.
In any case, the CMake comments by @tclune should be addressed by @gmao-jkolassa and @weiyuan-jiang .
cc: @biljanaorescanin @sdrabenh

Apologies for the obvious breakdown in communication.  Let me try to summarize today's discussion so far (based on the above and offline emails).  If I understand things correctly, merging the PR at this moment would:

facilitate GEOSldas (land-only) simulations with CNCLM4.0 and CNCLM4.5;
temporarily break the current capability to run the AGCM with CNCLM4.0 (Fanwei's science-tested version);
be only a stepping stone towards supporting Melanie's fire work (AGCM with CNCLM4.5).

To address 2. and 3., more work is needed per @smahanam 's comments (CatchCNRestarts.F90 and 2 perl scripts (preprocess_rst.csh and equivalent AGCM script).  Unless a higher priority is assigned to this, @smahanam would not immediately be available to help with this.
The question, mostly for @gmao-jkolassa, @rdkoster and @wmputman, is whether we should move ahead with the PR at this time.  The answer depends on whether there's a need for 1. and whether 2. and 3. are acceptable at this time.
In any case, the CMake comments by @tclune should be addressed by @gmao-jkolassa and @weiyuan-jiang .
cc: @biljanaorescanin @sdrabenh

@gmao-rreichle , to answer your questions:
Currently, we are actively running offline science experiments using both CNCLM40 and CNCLM45. These are the main motivation for developing this branch that supports both versions of CNCLM. So I would argue that there is a need for 1.
As far as I know, there are no experiments planned in the near future that would use CNCLM40 coupled to the AGCM. But @rdkoster would know better which CNCLM experiments are planned.
For Melanie's project, we are planning to run CNCLM45 coupled to the AGCM. This capability currently does not exist in any branch and would have to be added. If @smahanam is not available to work on this in the near future, would it make sense for @weiyuan-jiang and myself to work on this?
Unless there are some strong arguments against this, my vote would be to go ahead with this PR to have the support for the offline simulations and then aim to implement the CNCLM-AGCM coupling in a separate PR within the next 1-2 months.

@gmao-rreichle , to answer your questions:
Currently, we are actively running offline science experiments using both CNCLM40 and CNCLM45. These are the main motivation for developing this branch that supports both versions of CNCLM. So I would argue that there is a need for 1.
As far as I know, there are no experiments planned in the near future that would use CNCLM40 coupled to the AGCM. But @rdkoster would know better which CNCLM experiments are planned.
For Melanie's project, we are planning to run CNCLM45 coupled to the AGCM. This capability currently does not exist in any branch and would have to be added. If @smahanam is not available to work on this in the near future, would it make sense for @weiyuan-jiang and myself to work on this?
Unless there are some strong arguments against this, my vote would be to go ahead with this PR to have the support for the offline simulations and then aim to implement the CNCLM-AGCM coupling in a separate PR within the next 1-2 months.

We have no plans to use CNCLM40 coupled to the AGCM, at least not in the near future.

We have no plans to use CNCLM40 coupled to the AGCM, at least not in the near future.

For Melanie's project, we are planning to run CNCLM45 coupled to the AGCM. This capability currently does not exist in any branch and would have to be added. If @smahanam is not available to work on this in the near future, would it make sense for @weiyuan-jiang and myself to work on this?

If Melanie needs to run AGCM while I and Weiyuan are still buried in other stuff,  we sure can help set up CLM45 AGCM experiments for her project without the above updates.

For Melanie's project, we are planning to run CNCLM45 coupled to the AGCM. This capability currently does not exist in any branch and would have to be added. If @smahanam is not available to work on this in the near future, would it make sense for @weiyuan-jiang and myself to work on this?

If Melanie needs to run AGCM while I and Weiyuan are still buried in other stuff,  we sure can help set up CLM45 AGCM experiments for her project without the above updates.

For Melanie's project, we are planning to run CNCLM45 coupled to the AGCM. This capability currently does not exist in any branch and would have to be added. If @smahanam is not available to work on this in the near future, would it make sense for @weiyuan-jiang and myself to work on this?

If Melanie needs to run AGCM while I and Weiyuan are still buried in other stuff,  we sure can help set up CLM45 AGCM experiments for her project without the above updates.

For Melanie's project, we are planning to run CNCLM45 coupled to the AGCM. This capability currently does not exist in any branch and would have to be added. If @smahanam is not available to work on this in the near future, would it make sense for @weiyuan-jiang and myself to work on this?

If Melanie needs to run AGCM while I and Weiyuan are still buried in other stuff,  we sure can help set up CLM45 AGCM experiments for her project without the above updates.

My humble opinion is that this PR should wait until this new feature is available for the main modeling system (AGCM) before we merge it to develop. This slight delay should not have any impact on science research projects because naturally they get the highest priority - as you  know this PR followed Eunjee/Melanie's offline research. We will not wait for this PR if Melanie wants to launch her AGCM experiments.
Either way, I am not going to consider we are setting a precedent for future PRs where the feature is not supported by the main modeling system (AGCM) and all subsystems (GEOSldas).

My humble opinion is that this PR should wait until this new feature is available for the main modeling system (AGCM) before we merge it to develop. This slight delay should not have any impact on science research projects because naturally they get the highest priority - as you  know this PR followed Eunjee/Melanie's offline research. We will not wait for this PR if Melanie wants to launch her AGCM experiments.
Either way, I am not going to consider we are setting a precedent for future PRs where the feature is not supported by the main modeling system (AGCM) and all subsystems (GEOSldas).

@smahanam I think I have changed the file to make it consistent with the name changes but I will double check. However, I didn't know how to use perl script so I am not sure if they are compatible.

@smahanam I think I have changed the file to make it consistent with the name changes but I will double check. However, I didn't know how to use perl script so I am not sure if they are compatible.

@smahanam I think I have changed the file to make it consistent with the name changes but I will double check. However, I didn't know how to use perl script so I am not sure if they are compatible.

That wouldn't suffice Weiyuan. For e,g, CLM45 has number of new fields, number of column and pft dimensions are also different. Basically, mk_CatchCNRestarts.F90 does not know anything about CLM45 as it is.

@smahanam I think I have changed the file to make it consistent with the name changes but I will double check. However, I didn't know how to use perl script so I am not sure if they are compatible.

That wouldn't suffice Weiyuan. For e,g, CLM45 has number of new fields, number of column and pft dimensions are also different. Basically, mk_CatchCNRestarts.F90 does not know anything about CLM45 as it is.

@weiyuan-jiang Yes, all the modifications to support offline simulations with CNCLM40 and CNCLM45 were made and are subject to the PR. The modifications to run CNCLM coupled to the AGCM have not yet been implemented. Earlier this month @smahanam and I agreed that we would go ahead with this PR and the @smahanam would implement to AGCM coupling capability working off of the new 'develop'. This will temporarily break the ability to run CNCLM coupled to the AGCM, a combination that no one is currently using.
If it is possible to implement the AGCM coupling capability quickly, then I am happy to wait with the approval of this PR. However, it sounded like no one would be able to work on this anytime soon. So I am not sure about holding up the dual CNCLM support for the offline simulations until someone is available to work on the AGCM coupling. Just my 2c. If the preference is to wait with approving this PR, that's fine by me too.

@weiyuan-jiang Yes, all the modifications to support offline simulations with CNCLM40 and CNCLM45 were made and are subject to the PR. The modifications to run CNCLM coupled to the AGCM have not yet been implemented. Earlier this month @smahanam and I agreed that we would go ahead with this PR and the @smahanam would implement to AGCM coupling capability working off of the new 'develop'. This will temporarily break the ability to run CNCLM coupled to the AGCM, a combination that no one is currently using.
If it is possible to implement the AGCM coupling capability quickly, then I am happy to wait with the approval of this PR. However, it sounded like no one would be able to work on this anytime soon. So I am not sure about holding up the dual CNCLM support for the offline simulations until someone is available to work on the AGCM coupling. Just my 2c. If the preference is to wait with approving this PR, that's fine by me too.

Earlier this month @smahanam and I agreed that we would go ahead with this PR and
I am really sorry Jana, this is a misunderstanding - or I actually forget whether we were even talking about a PR.
If we strictly follow a protocol obviously this branch is not ready. If we want to operate as a one land team, that supports each other's projects, there is no issue at all,  We know we merged a bug, and we come back and fix it later.

the @smahanam would implement to AGCM coupling capability working off of the new 'develop'.
There is no issue here, I'll attend to this mid Feb.

Earlier this month @smahanam and I agreed that we would go ahead with this PR and
I am really sorry Jana, this is a misunderstanding - or I actually forget whether we were even talking about a PR.
If we strictly follow a protocol obviously this branch is not ready. If we want to operate as a one land team, that supports each other's projects, there is no issue at all,  We know we merged a bug, and we come back and fix it later.

the @smahanam would implement to AGCM coupling capability working off of the new 'develop'.
There is no issue here, I'll attend to this mid Feb.

@gmao-jkolassa @smahanam @weiyuan-jiang @gmao-rreichle @rdkoster, just a quick update from model development meeting, @wmputman said we should keep all on the feature branch until we can actually run experiments with AGCM both CatchCN CLM 4.0 and CatchCN CLM 4.5.
There will be no merge at this time.

@gmao-jkolassa @smahanam @weiyuan-jiang @gmao-rreichle @rdkoster, just a quick update from model development meeting, @wmputman said we should keep all on the feature branch until we can actually run experiments with AGCM both CatchCN CLM 4.0 and CatchCN CLM 4.5.
There will be no merge at this time.

Thanks for the update!

Thanks for the update!

My apologies Jana again for raising the issue that until the  feature is available for the AGCM, this PR may not be ready.

I updated the mk_restart utility to create AGCM restarts for both CLM40 and CLM45. This includes:
a) updates in mk_restart directory of this branch
b) updates in GEOSldas branch
c) created a new branch in GMAO_Shared using the same name - feature/jkolassa_cnclm45

There were a few things that had been overlooked in feature/jkolassa_cnclm45 in GEOSgcm_App - fixed that

merged the latest version of develop to this branch, also fixed a bug in GEOS_Surface

That all said, this branch is still not ready for sure. AGCM does not just run with both CLM40 and CLM45. There is a huge memory issue. NCCS kept revoking my node allocation each time I tried to run.

Sarith,
Thank you for the update and the changes you have made so far! Please correct me if I misunderstood the earlier conversation in January, but I was under the impression that you were working on the coupling with the AGCM. As far as I know no one else has been assigned to implement this.
What is still needed to add this capability?

Sarith,
Thank you for the update and the changes you have made so far! Please correct me if I misunderstood the earlier conversation in January, but I was under the impression that you were working on the coupling with the AGCM. As far as I know no one else has been assigned to implement this.
What is still needed to add this capability?

When I started working on this, I thought all I had to do is updating mk_restarts utility for the AGCM. After those updates, I started testing, fixed a minor bug and made some updates elsewhere too, and then I got stuck with this memory issue. I actually tried to investigate too (pretty much whole day yesterday), and realized this is beyond my limited skills.
OK then - I will work with Ben and fix this memory issue and what may come afterwards.
I am taking a day off today. That is also one of the reasons to throw the ball back to you. Anyway, this PR is not ready, I will work with Ben and make sure both CLM40 and CLM45 are available for the AGCM.

Got it, thanks! Please let me know if there is anything you need me to do. Enjoy your day off!

Hi Jana,
I think this is ready now for you to do some tests yourself.
I'll continue to help, so let me know if you run into any problem.

I made some  modifications yesterday. Please redo whatever you are doing from scratch.

I ran a few more tests and when I use
call MAPL_GetResource (SCF,  LAND_PARAMS,   label='LAND_PARAMS:',            DEFAULT="CN_CLM4",  __ RC __ )
LAND_PARAMS does not get changed to the new default. That is 'NRv7.2' is used even if LSM_CHOICE is 2 or 3. When I use
call MAPL_GetResource (MAPL,  LAND_PARAMS,   label='LAND_PARAMS:',            DEFAULT="CN_CLM4",  __ RC __ )
LAND_PARAMS gets correctly set to 'CN_CLM4' (for LSM_CHOICE=2) and 'CN_CLM45' (for LSM_CHOICE=3).
This seems to be counter to Sarith's earlier explanation on how the model should work, so I am not sure what the best way forward is.

Thanks for the tests, @gmao-jkolassa.   Note that the source code version of GEOS_SurfaceGridComp.rc has everything commented out.   In GEOSldas, a preprocessing script uncomments the lines that pertain to GEOSldas.  I don't remember how exactly @sdrabenh  addressed this for the AGCM when he recently addressed changing the defaults to the new land config.   Scott can probably help.

Thanks @gmao-rreichle, that is what I suspected. MAPL_GetResource(SCF...) finds a LAND_PARAMS in GEOS_SurfaceGridComp.rc, so the 'DEFAULT' set in the MAPL_GetResource statement is not used. It may be necessary to have different defaults in the surface resource file for the different LSM choices, but I would probably need some help to implement this.

LSM_CHOICE is the only parameter that the MAPL object knows - so that is the only configurable parameter via AGCM.rc by  passing MAPL  as an argument to MAPL_GetResource ( as it is in the develop version of the code). I just checked the code and verified.
So the SCF (Surface Config File) structure contains everything else (all surface parameters). MAPL object can't help reading them via GEOS_SurfaceGridComp.rc

Sorry I wasn't following your changes:
elseif (LSM_CHOICE.eq.2) then
call MAPL_GetResource (SCF, LAND_PARAMS,   label='LAND_PARAMS:',        DEFAULT="CN_CLM4",  RC )
elseif (LSM_CHOICE.eq.3) then
call MAPL_GetResource (SCF, LAND_PARAMS,   label='LAND_PARAMS:',        DEFAULT="CN_CLM45", RC )
else
is what you need. You used MAPL as an argument, MAPL did not know, the model used the default value.

Maybe, I have missed something. It looks to me you are good.

@gmao-jkolassa,
I wonder if it's really necessary to specify default values for non-default model choices.  As of now, the default model is Catch, and the default LAND_PARAMS is NRv7.2.  If someone wants to run CatchCN-CLM4 or CatchCN-CLM4.5, they need to change LSM_CHOICE from its default of 1 to either 2 or 3.  When they do that, they can also be expected to choose an appropriate LAND_PARAMS value.  Note that the code in SurfParams.F90 already protects against users forgetting this (the LAND_PARAMS default of NRv7.2 is not a permissible option with LSM_CHOICE=2 or 3.  So I think we're ok without further modifications.
There are probably ways to implement the appropriate defaults for CatchCN, and I'm not opposed to a smart solution, but I can't think of a simple and effective way just now.  (It's also late on a Friday...)
Having said that, I'm still wondering about the wisdom of not being able to access the SurfaceGC rc parameters in the MAPL object.  But that's a different discussion and not specific to CatchCN-CLM4/4.5.

@smahanam Just to clarify, using MAPL_GetResource(SCF...) will always set LAND_PARAMS to 'NRv7.2', which will cause problems in SurfParams_init if LSM_CHOICE is 2 or 3. So in its current for the code does not work for CatchCN.
@gmao-rreichle Currently, the user is not prompted to specify a LAND_PARAMS collection when setting up a GCM run. 'NRv7.2' is used by default, which causes the model to exit as per the check in SurfParams_init if LSM_CHOICE is 2 or 3. So, I think we have three options here: (1) add an LSM_CHOICE dependent default for LAND_PARAMS in the surface rc file, (2) modify gcm_setup to prompt the user for a LAND_PARAMS choice if LSM_CHOICE is 2 or 3, or (3) overwrite the specified LAND_PARAMS value with 'CN_CLM4' or 'CN_CLM45' inside the GEOS_SurfaceGridComp if LSM_CHOICE is 2 or 3. There may be another (better) option that I have not thought of.

I understand what you say Jana.
As of now
In Shared/SurfParams.F90
There is ONLY one parameter set for (LSM_CHOICE==2)  - just the default set of values.
There is ONLY one parameter set for (LSM_CHOICE==3)  - just the default set of values..
I am just wondering why do you need to worry today about an non-existing LAND_PARAM selection for when LSM_CHOICE > 1, in the first place.
You all know future of CLM40 and CLM45 better than me. Why can't we just delete select case statements inside else if (LSM_CHOICE==2) and else if (LSM_CHOICE==3) in SurfParams.F90  for now,  and deal with this extra option whenever you ever need it in the future (if that ever happens).
Note also that other  than GEOS_SurfaceGridComp.rc, there are 283 rc files to configure various runtime parameters for the model. AGCM.rc file is for the AGCM driver. So I am not sure I understand why SCF params should be part of MAPL.
In the past, I have only configured non-land parameters (some chemistry, GOCART params) twice. Both times, I edited the relevant resource file after creating the experiment following Chem folks instructions.
That is how others do it, that does not mean we should not explore other options.
Usually, I trust the default settings set up by various groups (GOCART/chemistry folks in this example) when I run the model for our work. So the same way, the land group's job is to get the default setting for the AGCM correct to avoid confusing non-land users. People who want to configure SCF, they actually know what they are doing and if we want to follow others, we can edit GEOS_SurfaceGridComp.rc in EXPDIR just like others do.
This my candid opinion, just take or leave --
One example from CO2: My note in AGCM.rc (based on instructions given by CO2 group), see how they change USE_CNNEE default value to 1 for the CO2_GC.
Again, take or leave...
Atmosphere-Land CO2 coupling
----------------------------
USE_CNNEE: 0 #  options : 0 (default); 1 (Chem uses NNEE from CatchmentCN, NOTE you must add USE_CNNEE: 1 to CO2_GridComp.rc, CMIP/CO
2_GridComp.rc,
MERRA2/CO2_GridComp.rc, MERRA2/19600101-20000331/CO2_GridComp.rc, MERRA2/20000401-present/CO2_GridComp.rc, and PIESA/
CO2_GridComp.rc
in /GEOSchem_GridComp/GOCART_GridComp/CO2_GridComp/ before you build the model.)

I am kind of signing off from this particular discussion. I will sure help you any other thing if need be, but these higher level discussions are not my thing.

@gmao-jkolassa:

Currently, the user is not prompted to specify a LAND_PARAMS collection when setting up a GCM run. 'NRv7.2' is used by default, which causes the model to exit as per the check in SurfParams_init if LSM_CHOICE is 2 or 3.

I see.  But as you can tell from my comments, I'm largely ignorant of gcm_setup.  That's really something that needs to be sorted out with @sdrabenh and whoever else is in charge of gcm_setup.

[...] three options [...] (1) add an LSM_CHOICE dependent default for LAND_PARAMS in the surface rc file, (2) modify gcm_setup to prompt the user for a LAND_PARAMS choice if LSM_CHOICE is 2 or 3, or (3) overwrite the specified LAND_PARAMS value with 'CN_CLM4' or 'CN_CLM45' inside the GEOS_SurfaceGridComp if LSM_CHOICE is 2 or 3.

I'm not sure how (1) would work without some additional annoying keywords and scripting.
I'm not sure the AGCM group would want to prompt for LAND_PARAMS, but perhaps that would be an option.  This prompt could be done immediately after prompting for LSM_CHOICE (assuming there's a prompt for LSM_CHOICE...).
In any case, the Surface GridComp rc file needs to be edited by gcm_setup (via script) or by the user (manually).  The latter is what I suggested yesterday.   Let the user select LSM_CHOICE 2 or 3 and at the same time expect the user to know that they need to edit the Surface GridComp rc file accordingly.  This could be communicated to the user during gcm_setup, should they select CatchCN.
Alternatively, gcm_setup could edit the LAND_PARAMS value in the Surface GridComp rc file.  For now, since there is only one possible value  of LAND_PARAMS for the two CatchCN flavors, gcm_setup wouldn't need to prompt for LAND_PARAMS and could just add the default LAND_PARAMS value into the Surface GridComp rc file.  Things get more interesting if there are ever two different versions of the same CatchCN flavor.
Viewed more broadly, the problem here is that the current setup only allows for one default land model, which per se seems reasonable to me.   CatchCN is not the default model by a long shot, so I think it's fair to expect CatchCN users to "know what they are doing" and edit the Surface GridComp rc file.  Or, again, work with @sdrabenh to see if gcm_setup can be edited.

If you scan programs in the directory tree  from GOESphysics_GridComp for the word MAPL_GetResource, you will see 649 call MAPL_GetResource lines.
Now take a look at AGCM.rc, and check number of parameters whose default values are actually defined via AGCM.rc (for those that are read from MAPL instance for e.g. DO_OBIO) - you will see just a very few (almost none).
So it seems, the AGCM's practice is setting default values  as a keyword inn call MAPL_GetResource line, while non-default values are need to know basis for those who have implemented. They change parameters in RC files after setting up the experiment. As I understand from my limited experience, that is how others configure parameters,
When we implemented the GEOS_SurfaceGridComp.rc, we understood ldas_setup works differently and land modelers actively work with different configuration etc., so we implemented different system for GEOSldas that you know how it works.
As part of design we follow below example using'=>" as a key for the script to turn on params.
#GEOSldas= >AEROSOL_DEPOSITION: 0
We also have lines like below
GEOSagcm=>AEROSOL_DEPOSITION: 0 - in my view at that time, it was merely documenting available configurable params  via GEOS_SurfaceGridComp.rc. More importantly, I was not even thinking to propose gcm_setup to implement what we do in ldas_setup. For gcm_setup  our RC file is just one of 283 RC files, and gcm_setup has been working  its own way for more than a decade.
The only important thing is this: Since  LSM_CHOICE: 3 is configured via AGCM.rc, then the default value of LAND_PARMS for LSM_CHOICE : 3 should be read from the default keyword in call MAPL_GetResource for the AGCM.
Just leave commented out paramteres in GEOS_SurfacceGridComp.rc as they are for the AGCM. I wouldn't propose to implement what ldas_setup is doing in gcm_setup.
My 2cnts...
In my mind,

Note that sometime in the next year, we will start migrating GEOS components away from ESMF config files and over to YAML files.     the <var>: <value> syntax works in both, but YAML does not have anything like the => syntax mentioned above.
In general the switch to standardized YAML is expected to make processing the resource files more straightforward as well as to allow other YAML-aware tools (e.g. Python) to work on the same resource files.   This is crucial for adopting workflows without dangerous duplication of configuration values across multiple files.
We expect the transition to be "gentle", allowing gridcomps to use either/both formats for some period of time before enforcing the abandonment of ESMF format.

Thank you all for the additional information, this is very helpful! It seems that the consensus is that users who choose CatchmentCN in gcm_setup can be expected to know that a different LAND_PARAMS collection is needed and can be expected to manually edit the GEOS_SurfaceGridComp.rc. for now. I think it would be helpful to make a minor edit to gcm_setup to include a reminder to update LAND_PARAMS if a user chooses LSM_CHOICE 2 or 3. I will work with @sdrabenh to implement this.

Thank you Tom for the heads up !
Jana, there is branch in GEOSgcm_App repo under the same name.
If you want to remind the user, open
https://github.com/GEOS-ESM/GEOSgcm_App/blob/feature/jkolassa_cnclm45/gcm_setup
and just after line 1164
if( $LSM_CHOICE == 3 ) then
echo "IMPORTANT: please set LAND_PARAMS: to whatever in RC/GEOS_SurfaceGridComp.rc file in the experiment directory."
endif
just write your message within "" in the echo statement. And please verify yourself.

Thanks, Sarith!

@tclune, just a quick note of explanation:

migrating GEOS components away from ESMF config files and over to YAML files. the :  syntax works in both, but YAML does not have anything like the => syntax mentioned above.

The => syntax is interpreted by ldas_setup and gcm_setup to pre-process the Surface GC rc source code file into a proper rc file in the run dir.  The resulting run dir *.rc file no longer contains "=>" and can be interpreted by both ESMF config and YAML.
The approach was developed because we need different default values of the Surface GC rc variables for the GCM App (ie, FP) and the GEOSldas App (ie, SMAP L4).  We also want to have just one Surface GC rc source code file to avoid maintaining two separate files that are otherwise identical in terms of the help text provided with the rc variables.
Please suggest alternative solutions if YAML offers any.

Processing such an isolated variant during setup rather than duplicating default resource files is sensible.     We can defer the exact mechanism by which this is managed until the setup scripts themselves are overhauled according to the "plan".   (Basically, we think we can write a generic setup script that applies across DAS, CTM, GCM, etc., but is itself configured by a resource file (will be YAML).   These resource files can indirectly embed some conditionals such as the one you are describing into the processing.    I'm wary of saying more until we have concrete examples and look to see what is the most intuitive/maintainable.

I have completed the science tests for this branch. The following experiments were conducted:
GEOSldas_CN40 - offline simulation using Catchment-CN4.0 forced with MERRA-2 meteorology
GEOSldas_CN45 - offline simulation using Catchment-CN4.5 forced with MERRA-2 meteorology
AGCM_CN40 - coupled simulation using Catchment-CN4.0
AGCM_CN45 - coupled simulation using Catchment-CN4.5
Outputs from all four experiments compare well against existing science-validated simulations using Catchment-CN4.0 and Catchment-CN4.5. Differences are within roundoff for the two offline simulations, for which reference simulations exist. Land outputs from the two coupled simulations are scientifically close to existing (offline) simulations.
From my end this PR is good to go.
Please note that in total 4 repositories need to be changed to allow for the support of the two different Catchment-CN versions. This PR is thus contingent on the following PRs
GEOSldas PR#326
GEOSgcm_App PR#177
GMAO_Shared PR#203

Great to hear all went well Jana !
When I introduced GEOS_SurfaceGridComp.rc, I used call ESMF_ConfigGetAttribute to read parameters from SCF everywhere.
Now that @tclune mentioned that " abandonment of ESMF" is in the plans and someone has already changed call ESMF_ConfigGetAttribute lines in GEOS_SurfaceGridComp.F90 to call MAPL_GetResource as well , this is just land software team's consideration.
When the SI team start working on ESMF to YAML migration, they will have one less thing to worry if we (land) could change remaining call ESMF_ConfigGetAttribute lines to call MAPL_GetResource  in the codes that the land group manages (note, there are similar lines related to GEOS_SurfaceGridComp.rc in GEOS_LandIce too).
Note : SCF = ESMF_ConfigCreate(rc=status) and call ESMF_ConfigDestroy   lines   (SCF, RC) should stay until the migration
I am not suggesting to include that change in this PR, though.  Maybe, it is also a good time to replace remaining old namelist files in GEOSldas repo.

Please be careful with statements like "abandonment of ESMF" in a public setting!  :-)   Just the resource files, not the framework.
(And it is likely that ESMF will extend interfaces to support YAML, in which case it is more like upgrading to a new type of ESMF resource file.)

My apologies Tom ! I misquote your earlier post.

@gmao-jkolassa, @biljanaorescanin, @sdrabenh,
I'm afraid we're not quite there yet.  There are a couple of minor issues and one bigger question:

mk_GEOSldasRestarts.F90 was moved from the GEOSldas repo to here by Sarith, but we still have three "mk_restart" programs with overlapping/duplicate elements (mk_GEOSldasRestarts.F90, mk_CatchRestarts.F90, and mk_CatchCNRestarts.F90).
My assumption was that someone would look at creating a unified "mk_restarts" preprocessor that works for both the AGCM and GEOSldas (where Catch and CatchCN could be in the same program or in different programs).
Unless this cleanup is executed, I don't see the wisdom of moving mk_GEOSldasRestarts.F90 from one repo to another because this move breaks the change history.
If we merge the PR as is, we will have missed an opportunity to clean up mk_restarts, which would be a shame and makes future maintenance and updates more difficult and error-prone.  I'm just stating this for the record.  I have no energy left to push for this.

mk_GEOSldasRestarts.F90 is full of hardwired Discover paths in the wrong form.  The paths should point to the official symbolic names of the directories.  Physical hard-drives (as in /gpfsm/dnb42) or links (as in ./rreichle/l_data/) should not be used.   A proper path would look as follows /discover/nobackup/projects/gmao/ssd/...    I talked with @biljanaorescanin about this, who cleaned up the paths as I was writing this comment.

The mk_GEOSldasRestarts executable lost its ".x" extension (presumably for consistency), but the "Usage ..." help text and comments.

Before the GEOSgcm_GridComp PR can be merged, we need to update and test the GEOSldas PR.  The GEOSldas branch is now out of date with the latest develop.  This can be done by @biljanaorescanin.

cc: @rdkoster @weiyuan-jiang

GEOSgcm_GridComp is the birth place of mk_GEOSldasRestarts.F90. Rolf/Weiyuan moved it to GEOSldas repo last year for some reasons. That program is needed for me to prepare CLM45 restarts for the AGCM add other capabilities to regrid.pl.
So it just came home.

"My assumption was that someone would look at creating a unified "mk_restarts" preprocessor:"
That was my plan as well when I started looking in to it. It needed some thinking, and I put some thought in to this too.
At the end, I decided to present something that works, supports both AGCM and GEOSLdas, more importantly good enough for the land software experts to understand the workflow of the regridding process that they had missed.
Since it was I who objected to this PR back in January, I just wanted to show land software experts what they had missed, what needs to be done to make it work.

Both Jana and I myself tested GEOSldas repo after moving mk_GEOSldasRestarts here, so that part has been verified.

Unified F90 code: good idea indeed, now that I have presented a system that works the way it should, please go ahead and upgrade. ScakeCatch.F90 and ScaleCatchCN.F90 can also be unified to a single program easily.
Alternatively, if you want, merge  to develop this working version that supports 2 CLM versions, and is available for both AGCM/GEOSldas systems for now - since it has been tested.
If you couldn't figure out yourself, I sure can do it later this year or in 2022. I many high priority projects on my plate now, sorry I am not available now.

@smahanam , when you move mk_GEOSldasRestarts from GEOSldas to here, I think the restart names  "catchcnclm40_internal_restart" or "catchcnclm45_internal_restart" become "catchcn_internal_restart".  Why do you change it? If there is no specific reason, I want to change it back.  ( I will do it)

Glad you noticed @weiyuan-jiang
When I started looking in to this, I realized the whole thing needed too many updates to bring it up to a working version that supports CLSM, CLM40 and CLM45 models in both GCM and GEOSldas. So I made that change to keep software/scripts simple and reduce the number of logics that we will need to add in order to accommodate 2 different file labels to get to a working version.
As it is, the model (I mean GEOSgcm.x or GEOSldas.x) uses 'catchcn' as the label for both catchcn40 and catchcn45 checkpoint/restart. By design, a model will always uses only one version for a simulation (CLM40, CLM45, CLM50) so using the same label should never confuse the model - i.e. no experiment is going to write both catchcn40_internal and catchcn45_internal during a single simulation.
Those who run the model know very well how to distinguish the version by looking at the netcdf header and they also know they are not interchangeable. Also, our scripts have the necessary checks before reading/writing an _internal_rst.
Anyway, this PR presents a complete  package of updates. Depending on long term plans of CLM40 and CLM45, if you want, please go ahead and update. Perhaps, the practical thing at this point  is let this working version see develop (and main repos) first, and address this label and other issues in a future PR.

Following the latest changes to this branch, I re-ran the science validation for this branch (as well as the associated branches in other repositories). I ran four science experiments in total.
CN_40: Offline simulation using Catchment-CN4.0 forced with MERRA-2 forcing data. Outputs from this run were within roundoff differences compared to the science-validated Catchment-CN4.0 simulation generated by Fanwei Zeng using LDASsa.
CN_45: Offline simulation using Catchment-CN4.5 forced with MERRA-2 forcing data. Outputs from this run were zero-diff compared to a Catchment-CN4.5 simulation previously generated using GEOSldas.
AGCM_CN40: Simulation using Catchment-CN4.0 coupled to the GCM. Outputs from this run were zero-diff compared to a simulation generated using the 'main' branch of GEOSgcm and selecting Catchment-CN4.0 as the land option.
AGCM_CN45: Simulation using Catchment-CN4.5 coupled to the GCM. No true reference currently exists for this simulation. Instead I evaluated the surface states and fluxes produced in this simulation against those produced in an offline Catchment-CN4.5 simulation (forced with MERRA-2). Differences were small and on the order that can be expected when comparing an offline and coupled experiment.
From my end, we are good to proceed with this PR.

Thanks, @gmao-jkolassa.  A quick question:

CN_40: Offline simulation using Catchment-CN4.0 forced with MERRA-2 forcing data. Outputs from this run were within roundoff differences compared to the science-validated Catchment-CN4.0 simulation generated by Fanwei Zeng using LDASsa.

Just to confirm: Is this offline run zero-diff w.r.t. a simulation using the current GEOSldas develop branch?

Just to confirm: Is this offline run zero-diff w.r.t. a simulation using the current GEOSldas develop branch?

Yes, it is.

@gmao-jkolassa, regarding the most recent commit 85b3070:
I take this change to mean that PRESCRIBE_DVG will not work for CatchCNCLM45.
The resource variable is still around for CatchCNCLM40, although I'm not sure it ever worked.
In any case, GEOS_SurfaceGridComp.rc probably needs some editing to point out the difference between CatchCNCLM40 and CatchCNCLM45 in this respect.
Are there other differences between CatchCNCLM40 and CatchCNCLM45 that need to be mentioned in GEOS_SurfaceGridComp.rc?
cc: @biljanaorescanin

@gmao-rreichle Yes, I was changing the ESMF_ConfigGetAttribute statements to MAPL_GetResource statements yesterday (this update had not made it into the CN GridComps because of the changed file names) and I accidentally added in PRESCRIBE_DVG, which has not previously been part of the CN_CLM4.5 GridComp. The latest commit was to correct that. It is unclear to me whether the initial decision to not have the PRESCRIBE_DVG option for CN_CLM4.5 was intentional or a mistake. So I can either add it in properly for CN_CLM45 or keep it in CN_CLM40 only and update the Surface GridComp resource file as you suggested.

@sdrabenh this PR is ready for review. No more updates will be pushed to this branch (except in case any updates to develop happen prior to merging this PR).
I've tested:

Develop when choosing catchment option vs this branch: it is zero diff
Develop when choosing catchment-CN option vs this branch option catchment-CN with CLM 4.0: It is zero diff.
This branch has a new option where one can chose catchment-CN with CLM 4.5. We don't have a reference AGCM run to compare it to. @gmao-jkolassa performed comparison of the branch and offline experiments to confirm only small differences (expected to be seen between the offline and coupled runs). The results were approved by @gmao-rreichle and @rdkoster .

The implementation of both Catchment-CN versions requires approval of the following linked PRs: GMAO_Shared PR 203, GEOSgcm_App PR 177. Once these are approved and merged, we will merge the updates in GEOSldas PR 326. .
I removed DNA label from all linked PRs.

@mathomp4: If you have any specific concerns or improvements re. CMake, perhaps write up an issue so this can be addressed in a separate, follow-up PR.  Thanks for going with the flow and letting this PR advance.  It's been around for a while and is complex enough as it were.  Merging it now is the sensible thing to do.  It may not be perfect, but it does what it needs to do for science and has been tested successfully.


Update Admin app to .NET 6

Having Hot Reload would really streamline our development workflow on the Admin App.

Jeff Fritz is going to take a first stab at this one at the hackathon


Dockerfile: switch to a ubi9 base

Build the image from a ubi9 base (requires a RHEL entitlement) rather than from Fedora.

Obsoleted by #8


SDCC and unused interrupts

Hi Georg,
I read with interest your recently-published guide to fault-tolerant software on the STM8 (https://github.com/gicking/STM8_Fault-Tolerant_SW). An excellent piece of work, well done! I have it bookmarked for future reference. 😄
An idea occurred to me while reading the section on handling unused interrupts.
You suggest one of the actions to take inside an unused interrupt handler is to trigger a reset. For that I had an idea of how that can be handled automatically, without any need to put in place code handling unused ISRs.
This may or may not be known to you, but when SDCC allocates how global and static variables are to be arranged in RAM, it never actually utilises the very first byte of RAM (at address 0x0). For some reason, the start of the DATA segment when linking is defined by default as being 0x0001 - you can see this in the .lk file generated upon compilation.
One other thing you may also not know is that any unhandled IRQs get assigned by SDCC an ISR address of 0x000000 in the vector table.
What my idea was is that this unused first byte of RAM at 0x0 can be taken advantage of as a de-facto "unhandled" ISR. By writing a byte value that is an invalid instruction (e.g. 0x75) to this location, whenever an unhandled IRQ then occurs, the following will happen:

CPU jumps to the relevant interrupt vector.
Reads and executes an "INT 0x000000" instruction from the unhandled interrupt.
Jumps to location 0x0 in RAM.
Reads and executes the illegal instruction 0x75.
An illegal opcode reset occurs.

The recently-implemented __sdcc_external_startup could be used to do the first-RAM-byte write, like so:
void __sdcc_external_startup(void) {
asm("mov 0x0, #0x75"); // Write illegal opcode (0x75) to first byte of RAM.
}
Or on older SDCC versions, just put it right at the top of main().
However... this is whole thing is caveated by the fact that in the vector table only the 'gaps' in the interrupt vector table have ISRs with an address of 0x000000 applied, and that the vector table only extends as far as the last handled ISR. The table will not be allocated beyond the last ISR, and what follows that last entry is the start of code. So if you have no ISRs at all, the table will only consist of one entry: the reset vector, and so you won't get these 'blank' entries.
So in order to handle unused IRQs in my proposed way, you'd need to create an interrupt routine in your code with the largest IRQ number, which I believe is 29. For example:
void dummy_isr(void) __interrupt(29) {
// Do nothing.
}
I don't think any STM8 devices actually have an IRQ 29, right? So I don't believe this would interfere with implementing any actual ISRs.
Anyway, what do you think? As it's just an idea right now, I have yet to test this on actual hardware. I have played around in the simulator, though, and it works in principle there.
Regards,
Basil Hussain

I created this issue only for easy reference in the main text.
Even though examples are for PlatformIO / SDCC, the hints themselves should not be toolchain specific. Therefore the above trick to use an special SDCC property is interesting, but I will only reference it in the main text.
Thanks anyway! I wasn't aware of this SDCC feature at all


Add B9 Aerospace HX and Legacy

Remove the name and release_status as this will be taken from KS, change the spec_version to 1. If you want you can add yourself to the author (optional).
For both files

Will do (when I get home).  Is there documentation on what the different spec versions mean somewhere?

I use the Spec.md to see which spec_version the file needs.

Fixed.

Merging with my thanks


Added Fix: Preventing to delete the Chapter Section.

This fix restores the deleted YouTube video chapters, allowing users to navigate specific segments of the video that were previously inaccessible.

Changes in addblocker() function. Operation on 'sponsor' is updated.


Fix version subcommands and generated output

Fix version command and flag -o which shows
version output in following formats 'yaml', 'json'
and 'short'.

I agree with @MalloZup here, I don't think we need to print the version in different formats.

@ereslibre, @MalloZup ok, then let's clean wrong version -output command usage

@mjura https://github.com/kubic-project/dex-operator/blob/cf315a5e044bf200b87c2ac0774ea5f188e5530b/cmd/dex-operator/main.go#L102 backport to the dex-operator :sunflower:

@MalloZup thank you for merging and as follow up to this https://github.com/kubic-project/dex-operator/pull/28


💡 Finished paste commands todo list

:)

Can you add what we discussed in discord with relation to history = 3 @Auxtal


Fix embed collaboration string and shrink NoM logo

Basic PR app checks out. Will wait 15 mins before merge in case there are comments.


Update README.md

Coverage remained the same at 100.0% when pulling 50d36675066d039ac1227356c1f4a0d544b1cda5 on update-readme into 8a5c53400245d29909083671bc732f19ccdcf5f0 on master.


Addition of Roadmap

@RitikPatni kindly look #107

Thank you.
On Mon, Oct 12, 2020 at 9:56 AM Ritik Patni<EMAIL_ADDRESS>wrote:

Merged #107
https://github.com/RitikPatni/Front-End-Web-Development-Resources/pull/107
into master.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/RitikPatni/Front-End-Web-Development-Resources/pull/107#event-3864617530,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/APXYEICLHWWUNMVICIHEYQDSKKANXANCNFSM4SGN6GHQ
.


Write up modified proposal.

I think this preserves the important properties of the Danezis proposal, while simplifying it slightly.
It also specifies a compact format for reports that allows extensible, freeform messages.  Each report includes a small amount of variable-length data in a memo field.  Depending on the length of the memo field, reports are between 134 and 370 bytes, or 70 and 326 bytes if user verification of source integrity is not important (i.e., if the server is trusted to verify reports were generated correctly).
The memo field includes a type byte, which can be allocated to applications.  For instance, we could define a schema like

0x0: "CoEpi symptom bitflags v1",
0x1: "CovidWatch test result" (maybe this is a 64-byte signature from a testing portal),
0xfe: reserved (can be used to add more than 256 types later),
0xff: binary data,

Rebased onto current main.
I'm going to merge this now; issues are welcome!


Create classes Email and Date in pregex.meta.essentials

Hi, are you already working on it? I can try adding an Email and Date classes if you want.

@dylannalex I've actually finished the Email one, though Date is yet to be made. You are welcome to try creating it, it first needs some thought on the design though. I'm thinking of it having a single string parameter "format",  through which you define the format of the date that you're willing to match, e.g. "mm/dd/yyyy". You can find more formats here: The other thing i'm thinking is that we could have Date(*formats), so you can match many formats with one instance. So, instead of one having to do:
from pregex import *

date1 = Date("mm/dd/yyyy")
date2 = Date("dd/mm/yyyy")

dates = op.Either(date1, date2)

they can just do:
from pregex import *

dates = Date("mm/dd/yyyy", "dd/mm/yyyy")

What do you think?

Hi @manoss96, I've been working on the Date class. Here's what I came up with:
Date formats
I added Date.date_formats, a Date class attribute that contains all valid date formats:
from pregex import *

Date.date_formats
>>> ('mm/dd/yyyy', 'dd/mm/yyyy', 'yyyy/mm/dd')

By default, Date matches any date format in Date.date_formats.
Date arguments
I followed your suggestion and let the user match many formats with a single Date instance.
from pregex import *

text ="""
01/11/2001
12/09/1996
1875/11/02
"""

pre1 = Date()
pre1.get_matches(text)
>>> ['01/11/2001', '12/09/1996', '1875/11/02']

pre2 = Date("dd/mm/yyyy")
pre2.get_matches(text)
>>> ['01/11/2001', '12/09/1996']

pre3 = Date("dd/mm/yyyy", "yyyy/mm/dd")
pre3.get_matches(text)
>>> ['01/11/2001', '12/09/1996', '1875/11/02']

Note: Date converts all uppercase characters in a date format into lowercase characters (e.g. "DD/MM/YYYY" is converted to "dd/mm/yyyy")
Invalid formats
The given formats are compared to date formats on Date.date_formats. When an invalid format is found, Date raises InvalidArgumentValueException.
pre = Date("dd/mm/yyy")
>>> pregex.core.exceptions.InvalidArgumentValueException: Provided date format "dd/mm/yyy" is not valid.

Let me know your thoughts. I'm up to adding more features or improving any aspect you consider!

I think this should also consider short-hand notations for years such as 02 for 2002. It might also make sense to add notations for time as well. Something like the strptime function in the datetime module.

@dylannalex Looks great, good job! As for the formats, I suggest that we follow this notation. That way we can have all lowercase while at the same time we can differentiate between 2002 and 02 like @alansun17904 said. For now, I'd say that implementing any valid combination of "d/dd", "m/mm", and "yy/yyyy" along with separators "/" and "-", is good enough. In the future, there can be more formats includes as well as the time. To wrap up, I suggest the following list of formats:

d/m/yy
dd/m/yy
d/mm/yy
dd/mm/yy
d/m/yyyy
dd/m/yyyy
d/mm/yyyy
dd/mm/yyyy
m/d/yy
mm/d/yy
m/dd/yy
mm/dd/yy
m/d/yyyy
mm/d/yyyy
m/dd/yyyy
mm/dd/yyyy
yy/m/d
yyyy/m/d
yy/mm/d
yyyy/mm/d
yy/m/dd
yyyy/m/dd
yy/mm/dd
yyyy/mm/dd

Plus all of the above using the "-" separator, suming to a total of 24 + 24 = 48 different formats.
I don't know about your current implementation, but I suggest having a dictionary of 6 different keys, namely "d", "dd", "m", "mm", "yy" and "yyyy", each mapping to a different pre-defined "Pregex" instance for matching each possible part of the date. Then it's just a matter of combining these instances together, separated by either "/" or "-". How's that sound?

Sounds great, @manoss96! Thank you and @alansun17904 for the help!
About what @alansun17904 said, I'd avoid date time values for now, since I consider it would be better to have a Date class for matching only dates and a Time class for matching time values. Once we have these two classes working, implementing a DateTime class should be as easy as merging Date and Time.

About what @alansun17904 said, I'd avoid date time values for now, since I consider it would be better to have a Date class for matching only dates and a Time class for matching time values. Once we have these two classes working, implementing a DateTime class should be as easy as merging Date and Time.

Yeah I agree with @dylannalex . As for the implementation that we discussed, feel free to use other classes from pregex.meta as they might help you. For example, you can use Integer(1, 12) for "m".

About default formats, it is impractical to hardcode all the 48 different combinations. What about adding an static method Date.date_formats() to compute all different format combinations. I think the itertools.permutations from the standard library would be a great tool for this task. Let me know if I can import this function!
Oh, and one last thing:

you can use Integer(1, 12) for "m"

Do you mean  Integer(1, 10)?

About default formats, it is impractical to hardcode all the 48 different combinations. What about adding an static method Date.date_formats() to compute all different format combinations. I think the itertools.permutations from the standard library would be a great tool for this task. Let me know if I can import this function!

Sure, you can use it. Just make sure that you import it with a different name starting with a "_" so it isn't directly imported every time pregex.meta is imported. Better yet, import it within the "Date" class itself.

Oh, and one last thing:
you can use Integer(1, 12) for "m"

Do you mean Integer(1, 10)?

Yeah I'm sorry you're right. I was under the impression that "m" matched "11" and "12" too, and that it only indicated that a single-digit month must not have a leading zero, e.g. "3" would be okay but "03" would not. In that case, I'm guessing using "Integer" would be an overkill so you can go with something simpler. However, if you find that some class in pregex.meta could help you, don't hesitate using it!

I've finished the Date class implementation. I've implemented each 48 different combinations dynamically, so adding new date formats should be straightforward.
Features:

If no format is provided, Date considers all possible formats.
All format provided are converted to all lower case (e.g. dD/mM/yyYY is converted to dd/mm/yyyy).
Raises InvalidArgumentValueException when an invalid format is provided.

I'm now working on documentation, which it's not my strong point. I'd really appreciate some help with it 😄
In a nutshell, the Date class has the following structure:
class Date(_pre.Pregex):
    '''
    Matches any date.

    :param str \*formats: Strings that determines which date formats to be considered a match.
        A date can either be dd/mm/yy, mm/dd/yy or yy/mm/dd (separated by by '/' or '-'), where:
            yy – two-digit year, e.g. 21
            yyyy – four-digit year, e.g. 2021
            m – one-digit month for months below 10, e.g. 3
            mm – two-digit month, e.g. 03\
            d – one-digit day of the month for days below 10, e.g. 2
            dd – two-digit day of the month, e.g. 02
        By default, all date formats are considered.

    :raises InvalidArgumentValueException: Invalid date format provided.
    '''
    __date_separators: tuple[str, str] = ("-", "/")
    __date_value_pre: dict[str, _pre.Pregex] = {
        "d":_cl.AnyDigit() - "0",
        "dd":_op.Either("0" + _cl.AnyDigit(), PositiveInteger(10, 31)),
        "m":_cl.AnyDigit() - "0",
        "mm":_op.Either("0" + _cl.AnyDigit(), PositiveInteger(10, 12)),
        "yy":_cl.AnyDigit() * 2,
        "yyyy":_cl.AnyDigit() * 4,
    }

    def __init__(self, *formats: str):
        '''
        Matches any date.

        :param str \*formats: Strings that determines which date formats to be considered a match. \
            A date can either be dd/mm/yy, mm/dd/yy or yy/mm/dd (separated by by '/' or '-'), where:
                yy – two-digit year, e.g. 21
                yyyy – four-digit year, e.g. 2021
                m – one-digit month for months below 10, e.g. 3
                mm – two-digit month, e.g. 03
                d – one-digit day of the month for days below 10, e.g. 2
                dd – two-digit day of the month, e.g. 02
            By default, all date formats are considered.

        :raises InvalidArgumentValueException: Invalid date format provided.
        '''

    def __date_pre(format: str) -> _pre.Pregex:
        """
        Converts a date format into a ``Pregex`` instance.

        :param str format: The date format to be converted.
        """

    def __date_formats() -> list[str]:
        '''
        Returns a list containing all possible date format combinations.
        '''

Looks good! Don't worry about documentation, I can do this later. A few points:

Make sure you do (cl.AnyDigit() - "0") in "mm" and "dd" so a match with "00" isn't possible.
Replace "PositiveInteger" with "Integer" as the former will try to match the sign "+" too.

After doing these I think that you're good to go, so open a PR whenever you're ready.

I think this seems great! I can help out with documentation as well if need be.

Thanks for your help, @manoss96!
PR is open. I've added tests and fixed what we discussed. I also ensured date values (i.e. 'd', 'dd', 'm', 'mm', 'yy', 'yyyy') are not enclosed by any other digit:
__date_value_pre: dict[str, _pre.Pregex] = {
        "d":_asr.NotEnclosedBy(_cl.AnyDigit() - "0", _cl.AnyDigit()),
        "dd":_asr.NotEnclosedBy(
            _op.Either("0" + (_cl.AnyDigit() - "0"), Integer(10, 31)),
            _cl.AnyDigit()),
        "m":_asr.NotEnclosedBy(_cl.AnyDigit() - "0", _cl.AnyDigit()),
        "mm":_asr.NotEnclosedBy(
            _op.Either("0" + (_cl.AnyDigit() - "0"), Integer(10, 12)),
            _cl.AnyDigit()),
        "yy":_asr.NotEnclosedBy(_cl.AnyDigit() * 2, _cl.AnyDigit()),
        "yyyy":_asr.NotEnclosedBy(_cl.AnyDigit() * 4, _cl.AnyDigit()),
    }

Greetings.

@dylannalex Played around with the class and it looks great. Good job. Since both "Email" and "Date" are done, I'm closing this issue.


docs: add debugging documentation for m3 records

Here is a debugging guide for m3 records. I kept it simple for now, but happy to modify/add to it if you see any issues. The rich text view is not rendering the image links correctly but they show up as expected when I hit preview in vscode:

So I'm fairly sure they are working, but let me know if you have concerns.

This looks great, thanks @SYU15!  I have a couple of very minor suggestions but I'd be happy to merge it as is, or with those tweaks.

thanks @SYU15 !


Request to Expose partition_unrolled Function for Public Use

Dear x86-simd-sort maintainers,
I am currently working on a high-performance sorting algorithm to handle billions of uint64_t data entries. To optimize the sorting process, I am leveraging parallel execution and the efficient sorting capabilities provided by the x86-simd-sort library.
In my implementation, I need to partition the data efficiently based on specific pivot values. The partition_unrolled function defined in src/xss-common-qsort.h appears to be highly optimized for this purpose. However, this function is not currently exposed for public use, making it challenging to directly utilize its capabilities.
Here is a simplified version of my parallel sorting implementation where efficient partitioning is critical:
extern "C" void parallel_sort(uint64_t * arr, size_t size, const std::array<uint64_t, 3>& pivots) {

    auto partition = [](uint64_t* arr, size_t low, size_t high, uint64_t pivot) {
        size_t i = low;
        size_t j = high;
        while (true) {
            while (i <= high && arr[i] <= pivot) i++;
            while (j >= low && arr[j] > pivot) j--;
            if (i >= j) break;
            std::swap(arr[i], arr[j]);
            i++;
            j--;
        }
        return i;
    };

    size_t mid1 = partition(arr, 0, size - 1, pivots[1]);
#pragma omp parallel sections
    {
#pragma omp section
        {size_t low_mid = partition(arr, 0, mid1 - 1, pivots[0]);}
#pragma omp section
        {size_t high_mid = partition(arr, mid1, size - 1, pivots[2]);}
    }
#pragma omp parallel sections
    {
#pragma omp section
        {x86simdsort::qsort(arr, low_mid, false);}
#pragma omp section
        {x86simdsort::qsort(arr + low_mid, mid1 - low_mid, false);}
#pragma omp section
        {x86simdsort::qsort(arr + mid1, high_mid - mid1, false);}
#pragma omp section
        {x86simdsort::qsort(arr + high_mid, size - high_mid, false);}
    }
}

Unfortunately, the custom partition function I wrote is quite inefficient, accounting for more than 60% of the total sorting time. This inefficiency diminishes the overall performance gains from parallel sorting.
To address this, I believe that making the partition_unrolled function available for public use would significantly enhance the library's utility and allow developers to achieve better performance in their sorting algorithms. Would it be possible to expose this function in the public API of the x86-simd-sort library?

I've currently figured out how to modify something to call it, but the code is getting rather ugly

@game-difficulty could you not just add __attribute__((visibility("hidden"))) to the partition function and build the shared library? That should expose partition function per your requirement.

Since the partition function is a template function, it might not work to export it with attribute((visibility("default"))) because template functions are instantiated only when used.

Have you tried building with this line removed: https://github.com/intel/x86-simd-sort/blob/87486d17a7d916f2992800b0219e28d0af29e765/meson.build#L42


[SPARK-33695][BUILD] Upgrade to jackson to 2.10.5 and jackson-databind to <IP_ADDRESS>

What changes were proposed in this pull request?

Upgrade the jackson dependencies to 2.10.5 and jackson-databind to <IP_ADDRESS>
Why are the changes needed?

Jackson dependency has vulnerability CVE-2020-25649.
Does this PR introduce any user-facing change?

No.
How was this patch tested?

Existing unit tests.

Can one of the admins verify this patch?

ok to test

Also, you need to update the manifest files:
./dev/test-dependencies.sh --replace-manifest

Test build #132390 has started for PR 30656 at commit 8e40796.

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36990/

Test build #132392 has started for PR 30656 at commit 7040f75.

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36990/

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36992/

Test build #132390 has finished for PR 30656 at commit 8e40796.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/36990/

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/132390/

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36992/

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/36992/

Test build #132392 has finished for PR 30656 at commit 7040f75.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/132392/


The ACMCSUF Logo disappears when navigating to a URL with multi-level subdirectory

What happened?
For example, when I go to https://acmcsuf.com/paths/ToTheMoon, the logo on the navigation bar is gone.

whoever wanna fix this probably just needa add a slash to the front of the url here
https://github.com/EthanThatOneKid/acmcsuf.com/blob/main/src/lib/components/sections/navbar.svelte#L34

cc @hhoang91

Could you please assign me to this issue?


Make sure that text is changed only from UI thread

A text which is attached to the Brick Text Editor should only be changed from within the UI thread. Otherwise we may have synchronisation problems

GtPharoSnippetCoder>>#updateAddOnsCommand is executed in a background thread (TaskIt). That update method requests an rbAst which accesses an editor text from the background thread. It may lead to race conditions if text is modified by the user from a UI thread. We should reimplement it

GtSourceCoder updates add-ons (updateAddOnsCommand) in a background thread. It is possible that one of the thing that changes in add-ons is a styler. In that case GtSourceCoder manually styles a text (see the screenshot). It takes a current text from the editor (which can be changed anytime from the other thread), styles and sets back in the editor. (edited)

In the meantime the collapsed editor does not have its own styler (edited) :

@j-brant: I think we need to create a styler for just the collapsed text instead of styling it directly.

I've added the styler for the collapsed text. Also, changed the calls to the editor's #styleText method to be from the ui thread as #styleText accesses the #text. Finally, I changed the methods that #isModified uses to not directly access the text as it may be called from background threads. With these changes, I no longer see any text accesses from non ui threads when opening the coders or when opening a debugger.

I believe we can close this one. We can open more specific ones when needed.


Child work items are not created. Instead a non-related requirements is created.

Steps to reproduce our issue:

Create a feature work item template either from scratch or a copy from a previous feature.
Initiate template duplicate and save the newly created feature
Click ellipsis and choose 1-click child links

It creates the child item, but it's pulling in a random Requirement that I don't see any correlation with. Even following the guide on here and other (not too many out there) various blogs and whatnot, I still can't 1. find the correlation to the child Requirement being created and 2. can't get it to create any child items no matter the tags I've set or other.
It's possible i'm not describing this well enough. Let me know what other details you need.

Maybe check the conditions using which the child items are created. Link
{
"applywhen":
{
"System.State": "Approved",
"System.Tags" : ["Blah", "ClickMe"],
"System.WorkItemType": "Product Backlog Item"
}
}


Fix required length for MACD

Current minimum length of max(fast, slow, signal) for MACD is incorrect and should instead be slow + signal because the signal is calculated based on the result of fast ema - slow ema. Also, since there is already a comparison between slow and fast that ensures that slow is longer, there is no need for max(fast, slow).

Hello @anthotsang,
Good catch! I will merge it into the development branch as soon as I can. Thanks for the contribution! 😎
Thanks,
KJ


Make isSupported() part of our public API

What
Make isSupported() public and export it in all.mjs
Why
So services can check whether GOV.UK Frontend is supported the same way our code does, reducing duplication.
Who needs to work on this
Developers, Content writers
Who needs to review this
Developers, Content writers
Done when

[x] isSupported() is made available in all.mjs
[x] isSupported() is documented as public in the JSDoc
[ ] The feature is documented in the Frontend Docs

Added an item to the Docs tasklist of the epic (#5205) so we can close this one.


Updated to Terraform Client 0.7 Updated docs

I encountered some issues while trying to run the demo so I'm sharing what I did to get up and running. PR: https://github.com/Capgemini/tf_tls/pull/46 should be dealt with first and references to the mattcoffey fork in main.tf should be reverted back to Capgemini.
N.B.: Only tested with Digital Ocean

hey @mattcoffey  thanks a lot for this, just dropped a few comments

Thanks for the comments @enxebre The odd stuff in inventory/inventory and comment about local ssh server was related to making the ansible part work without having specified the TF_VAR_STATE_ROOT properly previously. When I realised that was the problem it became obvious that it wasn't necessary to copy the IPs from the output of terraform apply or have a local ssh server running.

Hey @mattcoffey as part of the upgrade for terraform 0.7 we'd need to update the the templates to be "Data Sources" analogous to https://github.com/Capgemini/kubeform/pull/172/files#diff-f7c82420f5bbe654c1ba49edd173dab3R14
and to be consistent with the list format as per https://github.com/Capgemini/kubeform/pull/172/files#diff-530c171466f13a11a8a7c715564dbc4bR85
I'll wait in case you wanna add support for that to this PR, otherwise I'll merge this anyway as It's an improvement over what we have now.
Thanks!

I'll update the templates when I get back from travel on the 27th. Thanks for the pointers Alberto!


Can it be part of an audio recording program so that the user can separate the sounds and then record?

Can it be part of an audio recording program so that the user can separate the sounds and then record?

It's currently only meant for playback. In theory, you could extend it but this would require additional development.


Add new query params to allow better control of ApiStyleStore

I'm currently trying to integrate Maputnik into our platform and haven't been able to find a viable way to allow a non-technical map designer to work on a number of different styles without the need for a technical user level install or an offline, communication based process.
With these proposed changes, a link can be given to a non-technical map designer which allows them to modify a given map design which exists on a standard REST API endpoint that they would require access to.
This method also allows the decoupling of the CLI tool from the front-end which makes integrating the tool easier in more contexts as well. i.e. Services with Kubernetes.
By passing through the query params of styleApiProtocol, styleApiHost, styleApiPort and styleApiPath Maputnik can then access any REST API endpoint to get styles. At the moment, ApiStyleStore selects the first available style id, so I have also added the param styleApiPreselectedId which allows for the specific selection of style where more than one exist.
As an example:
https://maputnik.github.io/?styleApiProtocol=https&styleApiHost=example.com&styleApiPort=80&styleApiPath=%2Fapi%2Fstyles&styleApiPreselectedId=xxxyyyzzz

The above link will allow the editing of the style at https://example.com/api/styles/xxxyyyzzz as long as the endpoint implements typical REST measures.
Thought I'd put this forward to gather feedback before doing any further work. Any pointers with how this might be tested in the current suite would be great as well as how and where I should document this feature.

👋 @ianbelcher how about instead we pass in the URL as a whole, so
/?styleApi=https://example.com/api/styles

You can use the URL constructor to parse the URL, so
const urlObj = new URL(params.get("styleApi"));

@orangemug Will do. Just confirming that you're also happy with the naming of styleApiPreselectedId?
Where should this be documented as well?

Sorry for the delay. I'm a little confused what styleApiPreselectedId is doing. If you're loading a style from an API can't the thing you're loading from set the id? So you'd pass the following to Maptunik
/?styleApi=https://example.com/api/styles/xxxyyyzzz


Tech comm runtime upgr fix

Fix technical committee to bypass the weight checks (for the set_code function).

Am I closing this @zycon91 ? I don't see if it helps us anymore.

Am I closing this @zycon91 ? I don't see if it helps us anymore.

we can close it.


Unable to compile faster-rs on Windows

Hello, I'm trying to compile project which is intended to use faster-rs on my Windows 10 machine, I've installed LLVM-11.0.1 and cmake-3.20.3 and getting the following error during cargo build execution:
error: failed to run custom build command for `libfaster-sys v0.11.0`

Caused by:
  process didn't exit successfully: `E:\Rust\formult\target\debug\build\libfaster-sys-3fabb03800c04365\build-script-build` (exit code: 101)
  --- stderr
  CMake Deprecation Warning at CMakeLists.txt:3 (cmake_minimum_required):
    Compatibility with CMake < 2.8.12 will be removed from a future version of
    CMake.

    Update the VERSION argument <min> value or use a ...<max> suffix to tell
    CMake that the project does not need compatibility with older versions.

  CMake Warning:
    Manually-specified variables were not used by the project:

      CMAKE_ASM_FLAGS
      CMAKE_ASM_FLAGS_DEBUG
      CMAKE_BUILD_TYPE

  thread 'main' panicked at '
  command did not execute successfully, got: exit code: 1

  build script failed, must exit now', C:\Users\User\.cargo\registry\src\github.com-1ecc6299db9ec823\cmake-0.1.45\src\lib.rs:894:5

Any hints on how can I avoid this error?

Hi, this repo is not actively maintained + that faster-rs only works for Linux as stated in the readme.


fix: query command value from being reset to an empty state

Query form command values are being reset to an empty string whenever you navigate between two different Query actions(with different datasources) for example, Switching between Mongo query and S3 query.
Fixes #18353

Bug fix (non-breaking change which fixes an issue)

Manual

Test Plan

Add Testsmith test cases links that relate to this PR

Issues raised during DP testing

Link issues raised during DP testing for better visiblity and tracking (copy link from comments dropped on this PR)

Checklist:
Dev activity

[ ] My code follows the style guidelines of this project
[ ] I have performed a self-review of my own code
[ ] I have commented my code, particularly in hard-to-understand areas
[ ] I have made corresponding changes to the documentation
[ ] My changes generate no new warnings
[ ] I have added tests that prove my fix is effective or that my feature works
[ ] New and existing unit tests pass locally with my changes
[ ] PR is being merged under a feature flag

QA activity:

[ ] Test plan has been approved by relevant developers
[ ] Test plan has been peer reviewed by QA
[ ] Cypress test cases have been added and approved by either SDET or manual QA
[ ] Organized project review call with relevant stakeholders after Round 1/2 of QA
[ ] Added Test Plan Approved label after reveiwing all Cypress test

/ok-to-test sha=770c344

/ok-to-test sha=e0e276e

/ok-to-test sha=3439ccb


[GL300] SOS doesn't do anything

If I have the function key configured for SOS mode, the received message is treated as a normal position update, shouldn't  something appear in the info? When I check the logs the position received appears as a normal one.
Here is the initial part of the HEX message.
2b524553503a4754534f532c3330303530302c3836303539393030313136363531312c2c302c302c312c312c302e302c34342c3

If you need the rest let me know.

SOS messages are not supported yet, so they are decoded as regular location reports. Can I have full message sample?

That makes sense, here it is.
2b524553503a4754534f532c3330303530302c3836303539393030313136363531312c2c302c302c312c312c302e302c34342c37332e392c2d392e3832313136342c33382e3232303833312c32303136313030393133353335342c303236382c303030332c313737412c323435462c2c39362c32303136313030393133353335352c3139334424
Thanks

It might take some time before this is implemented, but thanks for the provided info.


Production deployment

It would be nice if the documentation included what to deploy to productions servers in terms of the configuration files.
Do we deploy any of them ? or just MVC is enough ?

You'd need Synthesis.config, MVC if using that, and also any configurations that you're registering in other patches.
Sorry, this got lost in my inbox :(

Added this note to #38


provide definition for 'narrow therapeutic index'

definition provided in the 2016-02-04 version of the OWL file.


Unable to add per-user remotes on snap version of lxd

Required information

Distribution: Ubuntu
Distribution version: 20.04
The output of "lxc info" or if that fails:

Kernel version: 5.15.0-53-generic
LXC version: 5.0.1
LXD version: 5.8
Storage backend in use: btrfs
Snap Version: 5.0/stable

Issue description

It is not possible to add remotes from the config.yml configuration file in the home directory located at ~/.config/lxd/ or ~/.config/lxc/ as mentioned at docs
The file specified with the environment variable LXD_CONF doesn't work either. Passing the config.yml configuration file using a variable LXD_CONF

Steps to reproduce

Install LXD vias snap (channel=5.0/stable)

with file:
2. create  config.yml in any of this dirs: ~/.config/lxd/, ~/.config/lxc/
3. run lxc remote ls to see no newly added remotes
with variable:
2. export lxd config variable export LXD_CONF=~/.config/lxc/config.yml
3. run lxc remote ls to see no newly added remotes

Information to attach

[ ] Any relevant kernel output (dmesg)
[ ] Container log (lxc info NAME --show-log)
[ ] Container configuration (lxc config show NAME --expanded)
[ ] Main daemon log (at /var/log/lxd/lxd.log or /var/snap/lxd/common/lxd/logs/lxd.log)
[ ] Output of the client with --debug
[ ] Output of the daemon with --debug (alternatively output of lxc monitor while reproducing the issue)

I tried to reinstall(remove && install) the lxd via snap, as a result I was able to see the remotes from the '~/.config/lxc/config.yml' via lxc remote ls, but after adding new ones or changing nothing happens.
Attempts to reboot the lxd daemon will lead to nothing. I tried both:

systemctl restart snap.lxd.daemon
systemctl reload snap.lxd.daemon

What did I miss?

With the snap, it's ~/snap/lxd/common/config/config.yml.

The reason why you saw it work once is because LXD will do a one time transfer from ~/.config/lxc to ~/snap/lxd/common/config/ on the first run for a given user. But it's a copy so modifying the original doesn't help.
LXD_CONF may work but it requires an absolute path, LXD doesn't perform shell expansion when processing paths, so putting in ~/ in LXD_CONF or LXD_DIR will either lead to errors or to a fallback mode.

Thanks, ~/snap/lxd/common/config/ work nice!


Investigate package size (consider sideEffect)

It would be useful to make sure our bundle size is as small as possible, and we are leveraging tree shaking as well.
It would be great to track our bundle size over time (with actual usage, not just full NPM size).
We might want to investigate the sideEffect NPM config.  I remember this being looked at for d3 and three.js years ago.  Also wasn't sure if it's only useful for webpack (and still applicable for rollup/vite)

https://github.com/d3/d3/issues/3076
https://github.com/d3/d3/issues/3131
https://github.com/mrdoob/three.js/issues/16059

Spent a little bit of time trying to look into the install and bundle size of Svelte UX and it's dependencies (with bundle being the most important).
Using pkg-size.dev provides some useful insights:

Shows the total install size as 149MB, with 109MB attributable to sveld, although the top graph shows the top dependency as typescript at 67MB.
Using bundlephobia.com show the bundle as 2.5kB minified, and the biggest contributor is self, followed by date-fns and lodash-es (which seems more reasonable for bundle size).

Using packagephobia.com, it doesn't provide much insight other than bundle size of 618kb and install size of 143MB

When I tried to use bundlejs.com, it didn't provide much useful information, but probably because I don't fully understand how to use it, especially with Svelte.

Using npm.anvaka.com it helps to show the dependency tree as a graph, with most of the dependencies comes as the result of @changesets/cli (everything not in the green outline).

While there isn't strong evidence from the above, I think the following could be helpful..

Remove lodash
Consider using unplugin-icons instead of using @mdi/js directly

Can we still access the raw path (not just the <svg>...</svg> (although that will still work with <Icon>)?
Will this break any change of running the components in the REPL (i.e. without Vite)?  Assuming this will still work with SvelteLab, StackBlitz, CodeSandbox, etc with our create-svelte-ux templates as long as they use our vite config.

Also took a look at a few small projects I've built with Svelte UX, and attemped to ignore the data/API portion

Site
DevTools
Download
Resources

434kB
687 kB

188kB
526 kB

97.2kB
287 kB

They all seemed reasonable.

Ran a couple quick lighthouse scores (for performance number), and they came back promising as well

Consider installing https://github.com/btd/rollup-plugin-visualizer (although based on PRs for Rollup v4, it doesn't look actively maintained at the moment).


[RFR] [OSF-8498] API: Add id and type to relationships

[#OSF-8498]
Purpose
Return relationship data for those relations of the form...
proto://host:port/v2//<namespace_id>/
Changes
Update api/base/serializers.py::RelationshipField::to_representation to add "data" to "relationships"
Side effects
This PR makes assumptions about the form of endpoint URLs and the naming standard of kwargs passed to views. I don't believe that failures to live up to my assumptions will result in returning bad data, it should result in no passed data, but bad data is a possibility.
Ticket
OSF-8498

Merged in https://github.com/CenterForOpenScience/osf.io/pull/7807