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

Naming consistency #1113

Closed
dopplershift opened this issue Jul 26, 2019 · 8 comments · Fixed by #1299
Closed

Naming consistency #1113

dopplershift opened this issue Jul 26, 2019 · 8 comments · Fixed by #1299
Labels
Area: Calc Pertains to calculations Type: API Change Changes to how existing functionality works
Milestone

Comments

@dopplershift
Copy link
Member

dopplershift commented Jul 26, 2019

Leading up to 1.0 it would be nice to standardize naming of function arguments, and eliminate any weird abbreviated names (similar to #1107). Things that stand out:

For kinematics.py:

  • lats: both abbreviated and plural when nothing else is. coriolis_parameter takes latitude.
  • thta in frontogenesis. At least should be theta. potential_vorticity_baroclinic uses potential_temperature. I think I prefer the latter.
  • height vs. heights. We don't have function arguments of temperatures, why do we have heights? Also, montgomery_streamfunction uses height

I'm sure more will pop out, but we need a good review. Changing these is technically an API break, but:

  1. I don't think many people pass these parameters by name
  2. If the name change, the code fails to run and you have an indication that you need to update your code (versus giving wrong answers)

My inclination is to just change these for 1.0 without worrying about a deprecation.

@dopplershift dopplershift added Area: Calc Pertains to calculations Type: API Change Changes to how existing functionality works labels Jul 26, 2019
@dopplershift dopplershift added this to the 1.0 milestone Jul 26, 2019
@zbruick
Copy link
Contributor

zbruick commented Aug 16, 2019

Taking a look at thermo.py:

  • dewpoint vs dewpt: extensive use of both, but I think dewpoint makes more sense. Also extends to derivatives of dewpt_start in lfc.
  • ref_pressure vs reference_pressure
  • rh vs relative_humidity
  • mixing vs mixing_ratio
  • heights: see above comment, it's in at least 3 functions in thermo
  • p vs pressure
  • theta vs potential_temperature
  • e vs vapor_pressure
  • part_press and tot_press to partial_pressure and total_pressure
  • Order of variables in specific_humidity_from_dewpoint: should be pressure first to match API
  • Also, dewpoint_from_specific_humidity has pressure last

Yikes.

I agree with just changing these without a deprecation. Error message that would result from these changes would be pretty clear to figure out from a user-perspective.

@zbruick
Copy link
Contributor

zbruick commented Aug 16, 2019

While I'm on it, here's basic.py:

  • wdir seems silly. Should be direction or wind_direction
  • geopot
  • rh
  • height
  • psfc and ptop

For cross_section.py:

  • There's a use of u_wind and v_wind, which should probably just by u and v.

For indices.py:

  • Update dewpt and heights if changed.

For tools.py:

  • Why is the function named lat_lon_grid_deltas, but input is ordered longitude, latitude?
  • heights vs height
  • input_dir to input_direction

@zbruick
Copy link
Contributor

zbruick commented Aug 16, 2019

Plots:

skewt.py:

  • p and t used in plot, plot_barbs, and other functions
  • w used as mixing ratio. This should likely just be mixing_ratio, given that w is reserved for vertical wind everywhere else in the codebase.

@zbruick
Copy link
Contributor

zbruick commented Aug 16, 2019

In addition to these updates in the code, I'm wondering if we should have something in the docs to indicate what the required variable names are for user input (or some sort of best practices) to better support community contributions: full words, no typical variable symbol names (i.e. no theta, although we allow u, v, w).

@dopplershift
Copy link
Member Author

Maybe put something under "Code Style" in the Contributor's guide? The only other thought that comes to mind is to put together a Developer's Guide, maybe renaming Infrastructure Guide and pulling out some thing from the Contributor's Guide.

@jthielen
Copy link
Collaborator

One more to add: in basic.py, geopot is used in geopotential_to_height (should probably be geopotential)?

@jthielen
Copy link
Collaborator

While this issue is more about function arguments, I noticed that the function dewpoint_rh is inconsistent with the rest of the library (following the existing standard, it should be dewpoint_from_relative_humidity). Should this be updated in v1.0 likewise, or should it go through a short deprecation cycle with the planned v0.12?

@dopplershift
Copy link
Member Author

@jthielen Since we can do that one with a deprecation cycle in 0.12, we should.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Calc Pertains to calculations Type: API Change Changes to how existing functionality works
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants