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

Enable linting in physics #48

Merged
merged 4 commits into from
Dec 6, 2021
Merged

Enable linting in physics #48

merged 4 commits into from
Dec 6, 2021

Conversation

elynnwu
Copy link
Collaborator

@elynnwu elynnwu commented Dec 4, 2021

In this PR, linting in fv3gfs-physics is enabled.

  • [TODO] Some of the constants in microphysics should be in a namelist config

Comment on lines 62 to 55
fv3gfs-physics/.*
)$
- id: flake8
name: flake8 __init__.py files
files: "__init__.py"
# ignore unused import error in __init__.py files
args: ["--ignore=F401,E203", --config, setup.cfg]
exclude: |
(?x)^(
fv3gfs-physics/.*
)$
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you'll need to do here is delete the fv3gfs-physics entry from the first exclude but leave the init.py one, and delete the exclude section altogether from the second entry. There should still be two flake8 entries.

Comment on lines -36 to -42
# - id: mypy
# args: [
# --no-strict-optional,
# --ignore-missing-imports,
# --follow-imports, skip # needed so we only test enabled files
# ]
# files: fv3gfs-physics
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should restore and uncomment this entry.

@elynnwu elynnwu enabled auto-merge (squash) December 6, 2021 19:15
Copy link
Collaborator

@mcgibbon mcgibbon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for enabling this! I hope it wasn't too much work to update all those constants references.


from fv3gfs.physics.global_constants import *
import fv3gfs.physics.global_constants as constants
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On one hand the math looks a little weird with constants everywhere, but on the other hand it's really helpful to be able to read the code and know what is a constant and what is a variable.

@@ -129,7 +129,7 @@
mono_prof = False # Perform terminal fall with mono ppm scheme
mp_time = 225.0 # Maximum microphysics timestep (sec)
prog_ccn = 0 # Do prognostic ccn (yi ming's method)
qi0_crt = 8e-05 # Cloud ice to snow autoconversion threshold (highly dependent on horizontal resolution)
qi0_crt = 8e-05 # Cloud ice to snow autoconversion threshold
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it OK to remove that from this comment? You could continue the comment on the next line.

@@ -1,6 +1,7 @@
from gt4py.gtscript import BACKWARD, PARALLEL, computation, interval

from fv3gfs.physics.global_config import *
from fv3gfs.physics.global_config import FIELD_FLT
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not about this PR but what is a FIELD_FLT and how is it different from a FloatField?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no difference actually. Let me clean this up.


# - pihom: homogeneous freezing of cloud water into cloud ice
# - This is the 1st occurance of liquid water freezing in the split mp process
# - This is the 1st occurance of liquid water freezing
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# - This is the 1st occurance of liquid water freezing
# - This is the 1st occurence of liquid water freezing

Comment on lines +33 to +34
description="fv3gfs-physics is a gt4py-based physical parameterization \
for atmospheric models",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: You're not wrong but this is the style I see us use most often.

Suggested change
description="fv3gfs-physics is a gt4py-based physical parameterization \
for atmospheric models",
description=(
"fv3gfs-physics is a gt4py-based physical parameterization "
"for atmospheric models"
)

@elynnwu elynnwu merged commit f13636e into main Dec 6, 2021
@elynnwu elynnwu deleted the fix/enable-physics-linting branch December 6, 2021 19:22
@elynnwu elynnwu restored the fix/enable-physics-linting branch December 6, 2021 19:25
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