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

Small fixes to python_dev_meeting branch #4

Merged
merged 3 commits into from
Feb 10, 2022
Merged

Small fixes to python_dev_meeting branch #4

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
16db43b
Introduce local variable in plon_type
billsacks Jan 30, 2022
9ef3901
Restore on/off options
billsacks Jan 30, 2022
d09113d
Fix some documentation
billsacks Jan 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions python/ctsm/args_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,10 @@ def plon_type(plon):
Returns:
plon_out (float): converted longitude between 0 and 360
"""
plon = float(plon)
if plon < -180 or plon > 360:
plon_float = float(plon)
Copy link
Owner

Choose a reason for hiding this comment

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

First, changing variable name from plon to plon_float is not a big deal and I can change this easily...

But I don't understand the motivation behind changing it and I want to clarify why...

  1. Python variable re-binding is allowed and in some cases saves memory.
    For example:
a= '5'
a = int(5)

is allowed in Python because it is a dynamic programming language.
Please see: https://stackoverflow.com/questions/48224911/is-it-good-practice-to-change-the-type-of-a-variable-in-python

In fact this (dynamic typing) is one of the features of Python, so why not use it?
https://www.pythontutorial.net/advanced-python/dynamic-typing-in-python/

  1. When the type of a variable changes it is recommended to use another name only when the old variable (with old type) is being used in the code again, which is not the case here...

  2. Now, let's look at this function closely. It is a function that is only used in the argparser type:

        pt_parser.add_argument('--lon',
                    help='Single point longitude. [default: %(default)s]', 
                    action="store",
                    dest="plon",
                    required=False,
                    **type = plon_type,**
                    default= 287.8 )

So the parser return to the user has a variable called plon (i.e. destination variable des=plon) with type float. So the user does not see/interact with string form of plon at all.... This is not a function that the user calls to change the variable for them rather it is a small helper function called by the argparse (with the method recommended by Python argparse).

  1. Finally, in Python it is always better to avoid using types in names to improve code readability... (hence the introduction of type hinting in python v3.6)...

Please check the accepted response here:
https://stackoverflow.com/questions/25489118/same-variable-name-for-different-types-in-python

  • Using type hinting, this is also 100% allowed :
def just_test(var: int) -> str:
    var = 'Printing: '+ str(var)
    print (var)
    return var 
 
# first var is integer:
var = 2 
var = just_test(var)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is all good discussion. I feel like for a different language I would know the answer for certain. And in FORTRAN for example it does often make sense to have the type in a variable name. Which is why my initial read is to endorse @billsacks suggested change. However dynamic scripting languages are completely different, and python has some real interesting subtleties to how it handles typing. So I think this is another thing that we should discuss as a group. I don't think this should be an issue, but we should discuss it, so I'll add it to an upcoming CTSM software meeting.

if plon_float < -180 or plon_float > 360:
raise argparse.ArgumentTypeError(
"ERROR: Longitude should be between 0 and 360 or -180 and 180."
)
plon_out = lon_range_0_to_360(plon)
plon_out = lon_range_0_to_360(plon_float)
return plon_out
4 changes: 2 additions & 2 deletions python/ctsm/config_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,9 +156,9 @@ def _convert_to_bool(var):
Returns:
var_out (bool): Boolean value corresponding to the input.
"""
if var.lower() in ("yes", "true", "t", "y", "1"):
if var.lower() in ("yes", "true", "t", "y", "1", "on"):
var_out = True
elif var.lower() in ("no", "false", "f", "n", "0"):
elif var.lower() in ("no", "false", "f", "n", "0", "off"):
var_out = False
else:
raise ValueError("Boolean value expected. [true or false] or [y or n]")
Expand Down
8 changes: 4 additions & 4 deletions python/ctsm/git_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

def get_ctsm_git_short_hash():
"""
Returns Git short SHA for the currect directory.
Returns Git short SHA for the CTSM repository.

Args:

Expand All @@ -30,7 +30,7 @@ def get_ctsm_git_short_hash():

def get_ctsm_git_long_hash():
"""
Returns Git long SHA for the currect directory.
Returns Git long SHA for the CTSM repository.

Args:

Expand All @@ -49,14 +49,14 @@ def get_ctsm_git_long_hash():

def get_ctsm_git_describe():
"""
Function for giving the recent tag of the git repo
Function for giving the recent tag of the CTSM repository

Args:

Raises:

Returns:
label (str) : ouput of running 'git describe' in shell
label (str) : ouput of running 'git describe' for the CTSM repository
"""
label = (
subprocess.check_output(["git", "-C", path_to_ctsm_root(), "describe"])
Expand Down