-
Notifications
You must be signed in to change notification settings - Fork 0
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
Small fixes to python_dev_meeting branch #4
Small fixes to python_dev_meeting branch #4
Conversation
This avoids changing the type of an existing variable.
I think these are needed for consistency with standard config file parsing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These make sense to me. Since, Python doesn't have a sense of type putting the type in the name seems appropriate to me. I also like adding the on/off option since we use that in other places.
This is getting tangential, but actually, part of my motivation for this was that python can have a sense of type (see https://docs.python.org/3/library/typing.html). @adrifoster and I have had some very preliminary discussions about moving towards using more type annotations in our python code. With or without that, though, it seems confusing to me to change the type of a variable within a function. |
@billsacks |
@negin513 I'm not seeing any review. |
@@ -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) |
There was a problem hiding this comment.
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...
- 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/
-
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...
-
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).
- 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)...
-
It is advised in Python to avoid using the types in variable names because it is potentially dangerous and reduces readability.
https://docs.quantifiedcode.com/python-anti-patterns/readability/putting_type_information_in_a_variable_name.html -
On the other hand, if there are two different variables with two types needed in the code by all means, it would have made sense to create
plon_str
orplon_float
or whatever names...
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)
There was a problem hiding this comment.
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.
@negin513 thank you very much for sharing your rationale with this detailed post. I can see how this would be a matter where there are opinions on both sides (which I guess is why https://stackoverflow.com/questions/48224911/is-it-good-practice-to-change-the-type-of-a-variable-in-python was closed as opinion-based). My feelings on this emerged from two places: (1) My experience working with large python code bases (mainly CIME), where I have found that reusing a variable name for a different purpose / type can make it more difficult to read and understand code. This function is small enough that it doesn't matter, but I'm thinking more about general rules here. (2) A long-term hope that we can have type hinting in our code along with using a checker like mypy. You pointed out that you can change the type of a variable and python will still run the code, but I don't think you can do this if you want to use static checkers like mypy (see https://adamj.eu/tech/2021/05/23/python-type-hints-mypy-doesnt-allow-variables-to-change-type/ ) or IDE features that leverage mypy or use similar techniques for checking code correctness. In fact, I'm not sure that python itself checks any type hinting for correctness: I think it is just intended for use with a tool like mypy (see https://www.blog.pythonlibrary.org/2020/04/15/type-checking-in-python , for example). If you run your code snippet through mypy, you get:
You raise some good points about the dynamic typing nature of python. This is a much bigger discussion that I don't want to get into right now, but I have come to feel that, for large and/or jointly-developed code bases, python's dynamic typing is a weakness rather than a strength, and that is why I hope we can move towards using type annotations together with a static type checker eventually. A few articles that express my views well are:
All that said, I'm fine deferring to you, @ekluzek and possibly @adrifoster if she wants to share any feelings on this. |
I tend to agree with @billsacks that changing the type of a variable can lead to confusion in large code bases. I think in this instance since the function is so few lines it's not a huge deal. I'll also say that even if you don't change the variable name (and just change the type), Python still has to create a new object/memory allocation under the hood, so I'm not sure how much memory we are saving one way or the other. Though in this instance I don't think it really matters very much since it is such a short function and small object types. I do really like the idea to start using type hinting. In terms of static type checking and using IDEs, I'll say that an IDE like Pycharm doesn't have an issue with I think the main benefit to using type hinting is that you know from looking at the function what the function needs and what you are getting out of it. I like the general rule of being explicit instead of implicit with coding. I also found this interesting stack exchange convo on dynamic vs. static typing: https://softwareengineering.stackexchange.com/questions/122205/what-is-the-supposed-productivity-gain-of-dynamic-typing For my part I like static typing, but this is likely because I use Fortran so much. I think in many cases we don't need to strictly follow all rules and guidelines if it makes more sense for our group to do something different. I'm happy to discuss this more at an SE meeting in the future. |
Thanks @billsacks for your response.
As someone said in the answers, this particular example is not about type casting at all: "The problem with your example isn't because of dynamic typing, but because you use the same variable to refer to three different things."
To quote yourself (and also re-iterating your point) this is probably more opinion-based. Python is a strongly, dynamically typed. The strong part refers to exactly this: "Strong typing means that the type of a value doesn't change in unexpected ways. A string containing only digits doesn't magically become a number, as may happen in Perl. Every change of type requires an explicit conversion." https://dev.to/icncsx/python-is-strongly-dynamically-typed-what-does-that-mean-5810 Again, I just want to clarify that I am all for type hinting which does not contradict with what I am saying. Type hinting does not mean adding |
Thanks for these thoughts, @adrifoster and @negin513. I should have said up-front that I am not at all tied to this variable name. I used the easiest thing that I could think of, but I didn't mean to argue that we should have a convention of using Hungarian Notation in any widespread sense, and I'm sorry if any of my comments gave that impression. I do think it's important to distinguish between two situations here, though: Convincing arguments I've read against using Hungarian Notation are ones that assume you are using it as a convention on every variable. I don't think those same arguments apply in the situation where you have two variables that represent the same thing other than their type: in that situation I feel like using the type in the variable name is often the most logical way to distinguish between the two variables. But again, I'm not at all tied to that if you can think of a more clear way to distinguish the variables. But truly, please do whatever you prefer here. I don't think this specific case is worth arguing over further, and I'm really fine with whatever you decide. I don't want to hold this up any further, so please go ahead and make any changes you want to my branch (you should have permission to push to it) or after merging my branch into yours. |
Store netCDF files via Git LFS
There were a few minor outstanding changes in ESCOMP#1461 that I thought would be easier to fix myself rather than comment on. See comments in the individual commits for details.