Skip to content

Tracmass for the isopycnic model MICOM#14

Draft
YanchunHe wants to merge 1 commit intoTRACMASS:mainfrom
YanchunHe:micom
Draft

Tracmass for the isopycnic model MICOM#14
YanchunHe wants to merge 1 commit intoTRACMASS:mainfrom
YanchunHe:micom

Conversation

@YanchunHe
Copy link

A draft pull request to merge Tracmass implementation for the isopycnic model MICOM

  • add MICOM project folder with configurations for reading model fields and grid

  • handle outcropping isopycnal layers (zero thickness)

  • handle north periodic boundary contion (north-folding)

  • shift u/v flux staggering of the C-grid from west-south to north-east

  • use model output vertical flux (w_explicit); or tracmass computed wflx (may still have error for some particles)

  • add water parcel depth as a tracer (defined as depth of layer interface; not accurate to be improved)

  • minor fixes of Tracmass code

add MICOM project folder with configurations for reading model fields and grid

handle outcropping isopycnal layers (zero thickness)

handle north periodic boundary contion (north-folding)

shift u/v flux staggering of the C-grid from west-south to north-east

use model output vertical flux (w_explicit); or tracmass computed wflx (may still have error for some particles)

add water parcel depth as a tracer (defined as depth of layer interface; not accurate to be improved)

minor fixes of Tracmass code
@joakimkjellsson
Copy link

Hi @YanchunHe

Thanks a lot for the contribution. I think it looks very interesting to include MICOM with isopycnal coordinates.

However, I'm a bit skeptical to approve this since it is all one commit with a lot of changes in one go. If the PR was just to add the MICOM project, we could do it, but the PR now includes changes which might affect other projects too and thus some testing of NEMO etc is required.

Also, in mod_post_tstep.F90 you have changed the behaviour from

IF (uu .GT. 0.d0) kb=ka+1
z1=DBLE(ka)

to

IF (uu .GT. 0.d0) THEN
    kb = ka+1
ELSE
    z1=DBLE(ka)
END IF

It's been a while since I worked on Tracmass, but it looks like this could change results for those not using explicit w.
And you have removed write_data('run') from mod_seed.F90 and changed the precision of some arrays in mod_tracerf.F90 (which is good, but a separate issue).

Could you split the PR into two PRs:

  1. Adding MICOM project
  2. Bug fixes. And please justify each bug fix, e.g. why is write_data('run') commented out etc.

Best wishes
/Joakim

PS. Future tip with git: Make a lot of small commits rather than one big one. For example, each bug fix, each style change, should be its own commit. It will make it much easier later on to trace what you have done and also to revert changes.

@YanchunHe
Copy link
Author

Hi @joakimkjellsson , thanks a lot for your feedbacks!

Yes, I agree that a commit/pull request for a specific bug fix/feature enhancement will be optimal. I have worked on this for a while, with close collaboration with @doos, and he is aware of what I have done.

Our hope is not to break any other model setup if there are changes in the main source code, which is why we use a predefined compiler flag. I did make some commits during my development, but these intermittent commits are mostly for my own tracking purposes, with quite a bit of debugging information.

This pull request is a working version that resolves all major issues that are needed for the isopycnic model, with some small bug fixes/formatting on the side.

Indeed, we plan to run further tests with MICOM and agree that it is good to test with NEMO to see if there are any unexpected side effects. And that is also why I made the pull request as 'draft', but it is not ready to merge.

I see the best way forward now is to add a few comments where I find I need to justify/explain my changes. I may also make some further small changes to the current pull request.

Hope this sounds OK?

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.

2 participants