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

Re-read the whole code, check everything #31

Closed
6 tasks done
phil-blain opened this issue Jun 1, 2020 · 11 comments
Closed
6 tasks done

Re-read the whole code, check everything #31

phil-blain opened this issue Jun 1, 2020 · 11 comments
Milestone

Comments

@phil-blain
Copy link
Owner

phil-blain commented Jun 1, 2020

  • check all comments to see if they make sense, they are useful, and do not denote things that still need to be done.
  • [ ] make sure we do as much code reuse as we can (especially for rheology computations) see Refactor rheology (stress) computations #36
  • remove unused variables
  • remove unused subroutines
  • [ ] check arguments vs use statements I did that a while ago, I won't change all interfaces again for now.
  • rename variables that have unclear names
  • compile with -Wall and fix warnings
  • fix whitespace inconsistencies
@phil-blain phil-blain added this to the Picard solver milestone Jun 1, 2020
@phil-blain
Copy link
Owner Author

I compiled with ice_dyn_vp.o: FFLAGS += -Wall added to Macros.conda_macos. This activates the -Wall flag only for the ice_dyn_vp.o target, i.e. when compiling ice_dyn_vp.F90.

This flag found

See 083b611, 71eba9e, 2a5f4b7, 7a8951a, 4931ac2

@phil-blain
Copy link
Owner Author

I fixed trailing whitespace issues, using git diff $(git empty-tree) cicecore/cicedynB/dynamics/ice_dyn_vp.F90 : 0d527f6

@phil-blain
Copy link
Owner Author

I fixed more whitespace issues, and uniformized whitespace for subroutine arguments and intents:
0179edf
7d46f1d
9b6c2bf
b32bdfe

@phil-blain
Copy link
Owner Author

I did some more code cleanup in 8218e82 and eadcfa6

@phil-blain
Copy link
Owner Author

I re-read every code comment in ice_dyn_vp and cleaned up a lot of them, also clarified others in 5ff03ab.

@phil-blain
Copy link
Owner Author

phil-blain commented Jul 16, 2020

Still need to :

  • clarify if the conv and r0 variables in FGMRES (and PGMRES) should be removed. I need to compare with the Saad implementation.

  • reread the FGMRES algorithm to find a better name and description for ww

@phil-blain
Copy link
Owner Author

I did more variable naming clean up in 1416a7e

@phil-blain
Copy link
Owner Author

I renamed ww to orig_basis in 9267229

@phil-blain
Copy link
Owner Author

I removed the conv and r0 arguments; they were unused. If we change the implementation in the future and we need them, we'll just re-add them.

@phil-blain
Copy link
Owner Author

there are no !phb comments anymore. I changed some of them to TODO.

@phil-blain
Copy link
Owner Author

phil-blain commented Jul 20, 2020

I went through the code and there is a lot of repetitions (copy-paste) between different computations regarding rheology.
Here is a list of the computations by subroutines:

calc_zeta_dPr (ice_dyn_vp)

  • computes zeta
  • computes \partial_x P_R, \partial_y Pr (derivatives of the replacement pressure)
  • computes `stressp_[1-4] (only P_R part)
  • then computes [cs]sigp[nsew12]
  • stress_Pr -> used in calc_bvec

stress_vp (ice_dyn_vp)

  • computes stresses from zeta
  • stress[p,m,12]_[1-4]
  • here stressp is the whole term

matvec (ice_dyn_vp)

  • computes (partial stresses) from zeta
  • stress[p,m,12]_[1-4]
  • stressp is only "linear" part (linear in \partial^n_[x,y] u)
  • computes [cs]sigp[nsew12]
  • computes str

formDiag_step1 (ice_dyn_vp)

  • computes strain_rates (with some coefficients = 0 or 1)
  • computes stresses (stressp -> only "linear" part)
  • computes Drheo (same computation as matvec, but special case for 8 corners...)

formDiag_step2 (ice_dyn_vp)

  • similar to the second step of matvec, bu without ccb (i.e. only ccaimp)

stress (ice_dyn_evp)

  • computes stress[p,m,12]_[1-4] -> different computations (EVP)
  • computes [cs]sig[p,m,12][nsew] -> SAME as matvec
  • computes str[pm]_tmp -> SAME as matvec

stress_eap (ice_dyn_eap)

  • computes strain_rates
  • computes deformations
  • computes [cs]sig[p,m,12][nsew] -> SAME as matvec
  • computes str[pm]_tmp -> SAME as matvec

We will refactor the code to unify these computations, but in a second step (after the Picard PR).

See #36

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

No branches or pull requests

1 participant