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.

Sign up for GitHub

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

Jump to bottom

Non-conformance in names #28

Open
jkrasting opened this issue Aug 10, 2022 · 7 comments
Open

Non-conformance in names #28

jkrasting opened this issue Aug 10, 2022 · 7 comments

Comments

@jkrasting
Copy link
Collaborator

@jetesdal and @gmacgilchrist - there are a lot of variables, attributes, methods, etc. that could have more descriptive names. As a general rule, they should be longer than 3 characters and be all lower case.

We can exempt a small number if there is a good reason, (e.g. ds is commonly used throughout the community to denote an xarray.core.Dataset object, but most of these could be made more intuitive.

Can you suggest some alternatives?

Variables -- Must be longer than three characters and all lower case

  • ct
  • dd
  • dm
  • dt
  • dy
  • F
  • F_mean
  • F_transformed
  • G
  • _G
  • Jlmass
  • l
  • p
  • sa

Attributes -- Must be longer than three characters and all lower case

  • Cp
  • da
  • dd
  • dl
  • ds
  • F
  • h
  • Jl
  • l
  • Ldot
  • lm
  • Qm
  • tf
  • ti
  • z

Functions -- Must be longer than three characters and all lower case

  • calc_F_transformed
  • calc_G
  • expand_surface_to_3D
  • hldot_from_Jl
  • hldot_from_Ldot_hldotmass
  • Jlmass_from_Qm_lm_l

Class Methods -- Must be longer than three characters and all lower case

  • calc_Fl
  • calc_F_transformed
  • calc_G
  • dd
  • F
  • G
@gmacgilchrist
Copy link
Collaborator

I wonder if retaining some of the capitalization might be important, to be consistent with mathematical nomenclature? For example, Lambda (or L) is the vertically-intergrated version of lambda (l); that is, capitalization denotes the exensive quantitity. Capital J is also a common mathematical notation for tracer fluxes, while capital Q is common for surface mass fluxes. These latter two are more easily replaced.

Anyway, here are suggestions for some of these...

Variables -- Must be longer than three characters and all lower case

  • ct : conservative_temp
  • dd : datadict
  • dm
  • dt
  • dy
  • F : @jetesdal Is this a functionally different quantity to hldot? I remain unsure about this.
  • F_mean
  • F_transformed
  • G : wmt
  • _G
  • Jlmass : jlambda_mass
  • l : lambda
  • p : pressure
  • sa : absolute_salinity

Attributes -- Must be longer than three characters and all lower case

  • Cp : specific_heat_capacity
  • da : @jkrasting Keep as da as another accepted terms for DataArray?
  • dd : datadict
  • dl : delta_lambda
  • ds
  • F
  • h : layer_thickness
  • Jl : jlambda
  • l : lambda
  • Ldot : @jetesdal @jkrasting Tricky one because the capital L is used to denote the extensive quantity (vertically inteegrated lambda). Capitalization is an accepted nomenclature, not sure what else to suggest.
  • lm : lambda_in_mass @jetesdal can you confirm that's what this is?
  • Qm : mass_flux
  • tf
  • ti
  • z

Functions -- Must be longer than three characters and all lower case

  • calc_F_transformed
  • calc_G
  • expand_surface_to_3D
  • hldot_from_Jl
  • hldot_from_Ldot_hldotmass
  • Jlmass_from_Qm_lm_l

Class Methods -- Must be longer than three characters and all lower case

  • calc_Fl
  • calc_F_transformed
  • calc_G
  • dd
  • F
  • G

@hdrake
Copy link
Collaborator

hdrake commented Apr 26, 2023

I wonder if retaining some of the capitalization might be important, to be consistent with mathematical nomenclature? For example, Lambda (or L) is the vertically-intergrated version of lambda (l); that is, capitalization denotes the exensive quantitity.

I agree with Graeme that it is more important for the capitalization convention for these terms to be consistent with the mathematical convention that the data formatting conventions, at least internally in the package.

I can work on implementing @gmacgilchrist's suggested renaming if others agree.

@jkrasting
Copy link
Collaborator Author

The Python programmer in me advises against this. The PEP-8 standard calls for lowercase variable names. The recommended way around this is to add descriptive suffixes, e.g. lambda_vint

@hdrake
Copy link
Collaborator

hdrake commented Apr 27, 2023 •
edited
Loading

Thanks for the input, @jkrasting. I hadn't known the convention was so strict for variables/functions.

The recommended way around this is to add descriptive suffixes, e.g. lambda_vint

I am satisfied with just using suffixes like this.

@gmacgilchrist
Copy link
Collaborator

Staying close to conventional standards feels like the right move.

@hdrake
Copy link
Collaborator

hdrake commented Apr 28, 2023

As a general rule, they should be longer than 3 characters and be all lower case.

Where does this convention come from? I don't see it in the PEP8 style guide.

@jkrasting
Copy link
Collaborator Author

You’re right, @hdrake. PEP-8 does not specify a minimum length, but some Python linters enforce this convention for clarity. Pylint is good example of this.

hdrake added a commit to hdrake/xwmt that referenced this issue May 3, 2023
@hdrake
Removed redundant swmt in favor of generalized wmt
5c6b080 
- Removed `swmt.py` and the `SurfaceWaterMassTransformation` class
within it, with the idea that all of its functionality is already
included within the core of `wmt.py` (and now `wm.py`).

- Moved the core (parent) `WaterMass` class to `wm.py`. This was
actually done in the last two commits, but here we include the
associated updates to the exporting in `xwmt.__init__.py`. The
keyword argument `t_name='temperature'` is now passed to `WaterMass`
in the analytical convergence tests, which had broken.

- Generalized core `WaterMassTransformations.datadict` method to
now identify surface fluxes as tendencies that do not have any
vertical coordinates (taken from `self.grid.axes['Z'].coords`)
in their dimensions. Addresses

- The vertical coordinate used in both calculations for pressure
(in the `wm.WaterMass.get_density` method) and for the 2D->3D
expansion that is currently used to compute WMT from surface fluxes
(see `compute.expand_surface_to_3d`) has been renamed to `z` (with
levels `z_l` and interfaces `z_i`) to emphasize that these only
support depth coordinates as currently implemented. For WMT calculations
in arbitrary vertical coordinates, these will need to be fundamentally
changed. (See related discussion at NOAA-GFDL#3).

- Added `/conventions/MOM6.yaml` file that can be read in as a
python dictionary that is formatted as required by the
`budgets_dict` argument of `WaterMassTransformations`, and
includes the default names of the core heat, salt, and mass
tendencies required for full WMT calculations in temperature,
salinity, and density space.

Issues addressed by this commit (or previous ones building up to)
- Makes many of the re-labellings requested in NOAA-GFDL#28

- Partially addresses NOAA-GFDL#13, although the information
is still stored as a dictionary. It should not be fairly straight forward to encode this information
in attributes of the data arrays in `self.ds`.

- Completely addresses NOAA-GFDL#7 and NOAA-GFDL#8,
I think, except for the expansion of 2D surface fluxes (see above).
There are now no redundant function or method definitions, as we
have removed `swmt.py` completely and made use of class inheritance.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@jkrasting @gmacgilchrist @hdrake