Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/restore severn #21

Merged
merged 14 commits into from
Aug 11, 2021
Merged

Feature/restore severn #21

merged 14 commits into from
Aug 11, 2021

Conversation

jpolton
Copy link

@jpolton jpolton commented Aug 5, 2021

Here are some code improvements and also restoring some things I broke (overwrote with SEAsia config edits). Thank goodness for version control...

In git one makes suggestions for edits by putting them on a separate branch and the creating a 'pull request'. The reviewers can then have a look at the changes ("Files changed") or checkout this branch and try it. Then can then make comments, suggest further changes, reject etc. the request.

When the reviewers are happy someone clicks "Merge Pull request" and the edits go into the master branch.

By working on branches and having reviewers, you limit opportunities to break the master. I should have got you into these habits sooner and avoided my mistake.

@mpayopayo mpayopayo merged commit f60c1a5 into master Aug 11, 2021
@micdom
Copy link
Collaborator

micdom commented Aug 12, 2021

@jpolton @mpayopayo
I've run everything again to check if this restored version is ok.
It is ok and working (unforced and with barotropic tides)!

But, I've noticed that
namelist_FES14.bdy has now ln_mask_file = .false. previously I had run it with .true.

Also, when running the barotropic tide run, now it produces only the SEVERN_baroT_1h_t.nc, while previously was producing SEVERN_baroT_1d_t.nc, SEVERN_baroT_1d_u.nc, SEVERN_baroT_1d_v.nc

@micdom
Copy link
Collaborator

micdom commented Aug 13, 2021

@jpolton @mpayopayo
the run was just too short, it produces also SEVERN_baroT_1d_t.nc, SEVERN_baroT_1d_u.nc, SEVERN_baroT_1d_v.nc

@mpayopayo
Copy link
Collaborator

@jpolton @micom
There is some inconsistency on the dates we ran, the tides call for 2005 but the model is 2000, I guess for now it doesn't matter.

I also noticed the ln_mask_file=.false. so I don't understand why we create the mask if we don't use it.

I don't have access to sn_src_msk = '/projectsa/NEMO/nibrun/ARCHER_DATA/INPUTS/AMM15/amm15_mask.nc' although that doesn't seem to give any problem

Would it be easier to have all the changes we include in the wiki on the namelist_FES14.bdy already on the file?

@jpolton
Copy link
Author

jpolton commented Aug 17, 2021

I turned the ln_mask_file = .false. because I released it wasn't needed for these experiments. (@mpayopayo it may yet be needed for time-varying boundary conditions so I wouldn't delete the method for its creation just yet).

@mpayopayo I don't think that the date inconsistency will matter for the tide only forcing but yes that is something to watch when time-varying forcing is used.

@mpayopayo it would make sense to put all the edits into namelist_FES14.bdy so that it can just be run without further edits. The associate wiki text can then either be deleted or left/editted as background information into the structure of the file should someone want to make their own changes (preferred).

@mpayopayo The "proper" way to make edits is to 1) create a new branch from the master. 2) make the edits there. 3) submit a pull request back to the master, so we can see the changes being proposed and all benefit from the updates. Maybe we need to do a tutorial on this as a Monday meeting thing, or something.

@jpolton jpolton deleted the feature/restore_SEVERN branch August 17, 2021 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants