Add method to write increment fields

[frld]

Description

This adds code to write increments. The existing state writing is converted to be (more) generic (in particular with the filename supplied by a string) and then this is called in separate routines suitable for state fields and increment fields.

Also I've found previously having this generic writing code available is useful for generating output files for debugging.

NB this PR follows on from PR #88 (where the overall PR plan is listed).

Issue(s) addressed

N/A

Dependencies

List the other PRs that this PR is dependent on:

• N/A

Impact

No expected impact

Checklist

• I have updated the unit tests to cover the change
• New functions are documented briefly via Doxygen comments in the code
• I have linted my code using cpplint
• I have run the unit tests
• I have run mo-bundle to check integration with the rest of JEDI and run the unit tests under all environments

[frld]

Toby,

Another PR for you to look at if that's OK. It builds but I haven't completed the testing because I wanted to check something first...

In addition to what is presented I was wondering about renaming StateIOUtils* to FieldsIOUtils* (say) and move them to a new subdirectory called Fields or maybe IO because it (now) looks a bit funny having increments writing code in state. The new routines writeStateFieldsToFile and writeIncrementFieldsToFile (perhaps renamed) could go into State and Increment. Any other suggestions welcome.

NB I'll do the doxygen and unit tests once we've decided where this new code will go.

Thanks

Dan

[twsearle]

OK since it isn't yet ready for review, I have switched it back to draft.

I am happy for you to move those functions to a separate directory if that makes the most sense.

I was also wondering would you be willing to use the Parameters class, as used elsewhere in orca-jedi (e.g in State), to make explicit the parameters used in the configuration of increments? It makes reviewing and documenting the requirements of the interface easier, and allows for checking of the configuration file using a json schema when writing the yaml configuration file.

I am a bit confused about why you would need an "unset" type? Any existing atlas field has to have a type at the time of creation on my understanding. So far we have been specifying the type as part of the configuration and I am concerned that this turns the FieldPrecision object into a MaybeFieldPrecision object.

[frld]

I am happy for you to move those functions to a separate directory if that makes the most sense

OK thanks, I'll implement something and you can let me know what you think. It wouldn't be too difficult to move things again if it doesn't look the best idea.

I was also wondering would you be willing to use the Parameters class, as used elsewhere in orca-jedi (e.g in State), to make explicit the parameters used in the configuration of increments? It makes reviewing and documenting the requirements of the interface easier, and allows for checking of the configuration file using a json schema when writing the yaml configuration file.

Yes. When I first did this I wasn't quite clear on how this worked so it seemed simpler just to do it the way I have. However you have convinced me of the benefits. I'll attempt this as part of the PR.

I am a bit confused about why you would need an "unset" type? Any existing atlas field has to have a type at the time of creation on my understanding. So far we have been specifying the type as part of the configuration and I am concerned that this turns the FieldPrecision object into a MaybeFieldPrecision object.

This is only because I want to have an optional parameter in the writing routine which allows you to specify the type. I need this because the increments are always double (even when the equivalent state variable is a float). I.e for the increments geom.fieldPrecision(fieldName) gives the wrong type. (See ~line 179 in StateIOUtils.cc). In python you can specify a default value of None but I didn't think C++ had something similar. Another way of doing this I can think of is to have a string optional argument.

Alternatively is there a way it could query the atlas field to get the type rather than using geom.fieldPrecision? That would be nice as it would bypass all this and make writeFieldsToFile automatically more generic.

[twsearle]

The datatype method returns an object that gives the type of the field if you need it.

However I think there might be a better way. I might be misunderstanding, but if you have some yaml configuration where you are specifying the required type to write you can use the ParameterTraitsFieldDType (move it to utilities/somewhere else if shared between classes) to specify the type at configuration time.

So far we have been using optionals over null types. I personally think this is better, but I accept it is a bit of a personal preference thing! One thing I will say though, is that our philosophy so far has been to push as many errors into being checked at compile time, configuration time, or during the construction of the various objects in oops (the constructors tend to run before any processing). This allows us to check validity and abort a program early in the execution when we know the results are invalid/not what was intended, so that the user can fix things and rerun quickly.

[frld]

Thanks Toby.

I certainly don't want to deviate from the style of coding you are using orca-jedi, if for no other reason than to avoid putting a extra maintenance burden on you and your colleagues. I'm expecting to have to regig my suggestions as in many cases I will have selected the first way I could think of to get something to work. Any I think I have enough info work on this PR and move it out of DRAFT mode. I don't know if it will send you an email but I'll also let you know when I'm ready for review.

BTW it looks like atlas datatype is quite a neat way of doing things but perhaps there are issues I have not anticipated.

[twsearle]

The atlas datatype object is ok, but in my opinion it is very fortran-centric with its datatypes (real-32 etc). Since it is a goal of the JEDI project to use C++ exclusively over time, I am a bit reticent to introduce fortran types. On top of that, inside actual atlas the work takes place in c++, so the true types of the data are c++ types. Having said all that I have had to make use of them as part of atlas dev.

[frld]

Hi Toby,

I've now revived this PR which is to add an increments write method. Since you last looked I've implemented the use of parameters for the increments configuration settings. And I've also added appropriate unit tests. I've added comments to document the new functions. The orcajedi c tests pass. I'm presuming the automated check will cover mo-bundle integration test requirement (I'll run it manually if this is not the case).

I know the use of atlas datatypes is a potential sticking point - if this is the case perhaps seeing the completed PR will make it easier to see a different solution.

Thanks
Dan

[twsearle]

Hi Toby,

I've now revived this PR which is to add an increments write method. Since you last looked I've implemented the use of parameters for the increments configuration settings. And I've also added appropriate unit tests. I've added comments to document the new functions. The orcajedi c tests pass. I'm presuming the automated check will cover mo-bundle integration test requirement (I'll run it manually if this is not the case).

I know the use of atlas datatypes is a potential sticking point - if this is the case perhaps seeing the completed PR will make it easier to see a different solution.

Thanks Dan

Thanks!

The integration testing using mo-bundle is something we perform separately to check everything works on our HPC systems with the deployed compilers. To run the mo-bundle tests, you need to reference the branch from this PR when running bbb.

1. change to the root directory of mo-bundle
2. make sure mo-bundle is up to date by running git checkout -- ./*; git pull
3. run ./bin/bbb ctest <your-chosen-workflow-name> -S 'REF_ORCA_JEDI="feature/increment_write"' to run mo-bundle using the branch from this PR.
4. paste the link to the cylc-review output in the ticket description.

Further info on bbb is in the mo-bundle documentation visible on the repo homepage.

Once these tests run through, I will find some time to give this change another review, hopefully later this week.

[frld]

Thanks Toby. No problem waiting a bit if you are too busy.

On the integration tests - I found the git checkout -- ./*; git pull command didn't work for me. Instead I updated mo-bundle and used the script bin/update-repos. After doing that I ran the integration / bbb test and I claim success in this. The cylc output is here http://fcm1/cylc-review/cycles/frld/?suite=pr89_increment_write . (There was one task that failed, but this ran out of wallclock and succeeded on being retriggered).

[frld]

Updated integration test results here http://fcm1/cylc-review/cycles/frld/?suite=pr89_increment_write2

[twsearle]

I had another little look at this change and I have a question. I am still putting together my thoughts, but from what I can see, an increment variable has to have the same name as its corresponding field variable at the moment. Is that correct?

[frld]

I had another little look at this change and I have a question. I am still putting together my thoughts, but from what I can see, an increment variable has to have the same name as its corresponding field variable at the moment. Is that correct?

I don't know for sure. I think generally that might seem sensible that a particular increment is linked to particular state variable but I haven't done anything to enforce it yet in my changes. I wasn't sure of the impact of linear variable changes which we will need for nemovar.

Is there something about this PR which might cause additional issues perhaps because I'm plugging into routines you've written for writing state variables for writinng out the increment? Or is it a more about the overall plan and the code I added in the previous PR?

Currently all my DA experiments with JEDI have been single variable ones using the same state and increments with the same name. I'm thinking now I will add a very basic 3DVar test to the next PR and I could investigate this issue more then. (Or I could do more in this PR as you prefer).

I can add into the plan a PR which implements a simple multivariate assimilation which will be a good test of these aspects. That also might be a good time to look at implementing any changes based on the jcsda discussion you linked me to.

[twsearle]

@frld I wasn't really thinking anything in particular, more about the broader design of this sort of stuff (I am still understanding how the increments/dirac configs fit into everything). In the past there have been distinctions between a lot of different lists of variables, some of which need to be subsets of others. The increments in orca-jedi at the moment would fail if an increment variable is made that isn't listed in the Geometry. I am not actually sure if that is what we want long term with linear variable changes. Either way we can handle it, I just thought it was worth asking in case you knew :)

[twsearle]

This is all looking really great thanks! I am doing my best to understand every part so that we can get things as simple and coherent as possible - sorry as this does make our reviews quite long at least for the first few!

I put together a branch containing some suggestions based on the discussions in the conversation on this PR along with outcomes from my review. Consider those reviewer suggestions, but if you would like to merge them into this branch directly feel free:
#105

[frld]

@frld I wasn't really thinking anything in particular, more about the broader design of this sort of stuff (I am still understanding how the increments/dirac configs fit into everything). In the past there have been distinctions between a lot of different lists of variables, some of which need to be subsets of others. The increments in orca-jedi at the moment would fail if an increment variable is made that isn't listed in the Geometry. I am not actually sure if that is what we want long term with linear variable changes. Either way we can handle it, I just thought it was worth asking in case you knew :)

Thanks that's useful to be made aware of. I'll keep this issue in mind when getting onto the linear variable change work (but dodge it for now!)

[twsearle]

Thanks very much for this contribution the help with understanding more about Increments!

[frld]

Thanks Toby. The fun will continue here #106 (draft PR for now) which will try to start doing (very simple) assimilation with orca-jedi.