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

Refactor code py3.10 xyluo25 #146

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

xyluo25
Copy link

@xyluo25 xyluo25 commented Feb 12, 2024

Hi @martinfleis,

I re-forked the main repository to avoid version confusion. By the way, thank you for your recommendation from last pull request. I understand that Sphinx accepts both NumPyDoc and Google Docstring styles, given Google Docstring's broader acceptance, I've been using it for new functions in previous commits. From your comments, it seems you're advocating for consistency in the documentation style. Acknowledging your point, I will also adhere to the NumPyDoc style to maintain this consistency.

Three documents modified in this request.
utils.py: add type hints from master branch
summary.py: add type hints from master branch
sel_bw: add type hints, refactored codes, add code comments

Please check these changes again and looking forward to your suggestive feedback.
Xiangyong Roy Luo

@jGaboardi
Copy link
Member

@xyluo25 We really do appreciate your contributions! I think one of the main points that @martinfleis is trying to make here is that we should all discuss proposed changes and agree on scope before work begins.

While this current PR certainly is much more reviewable than #144, we still would ideally discuss the proposed changes in an issue and get the lead maintainers's opinion (@Ziqi-Li). And this can be done iteratively.

We deeply value the time commitment from all contributors (especially newcomers) and want that experience to both be rewarding and constructive!

@xyluo25
Copy link
Author

xyluo25 commented Feb 13, 2024

@xyluo25 We really do appreciate your contributions! I think one of the main points that @martinfleis is trying to make here is that we should all discuss proposed changes and agree on scope before work begins.

While this current PR certainly is much more reviewable than #144, we still would ideally discuss the proposed changes in an issue and get the lead maintainers's opinion (@Ziqi-Li). And this can be done iteratively.

We deeply value the time commitment from all contributors (especially newcomers) and want that experience to both be rewarding and constructive!

Hi @jGaboardi, thank you for your constructive suggestions. I am currently a Ph.D. student at ASU, working in part with Professor Stewart Fotheringham. After discussing with Professor Fotheringham, we have decided to upgrade the current codebase to a newer version (Python 3.10 or higher). Additionally, we are considering the inclusion of more functions into the package in the future. I am truly grateful for @martinfleis's invaluable feedback and am learning a lot from his insights. Our goal is to continually update the package and contribute to the open-source community, serving a broader spectrum of developers.

@jGaboardi, I totally agree with your comment: "We should all discuss proposed changes and agree on the scope before work begins.". I have attached the document outlining the proposed changes to my initial pull request, which were confirmed after consultation with Professor Stewart Fotheringham. However, I believe a further confirmation from @Ziqi-Li regarding these changes is still necessary. Please refer to the attached document for details of changes.
MGWR python package change list.pdf

@martinfleis
Copy link
Member

Thanks for the additional context, you should've stated that early on :).

In this case, I'll let @Ziqi-Li and @TaylorOshan handle your PRs. All my comments still apply though, including opening issues in the repository to discuss the changes before opening a PR implementing them.

@Ziqi-Li @TaylorOshan @jGaboardi ping me if you'll need my input here.

@xyluo25
Copy link
Author

xyluo25 commented Feb 13, 2024

Thank you @martinfleis. I will follow your comments by opening issues in the repository to disscuse possible changes first.

@Ziqi-Li
Copy link
Member

Ziqi-Li commented Feb 14, 2024

@xyluo25 I have reviewed each line of the change. Just to help summarize what you have done:

  1. added type hints for some functions
  2. changed some if-else to one line
  3. added some comments for removing variables that are not used.
  4. updated assertAlmostEqual() in the unittests
  5. added some space before hashtag for comments, or arguments

What I see is that 4. is essential, 3 and 5 are optional. And I personally think 2 will reduce readability. For 1, I am not a big fan of using type hints (I know they are useful) because they make defining functions look overly complicated, slightly unfriendly to new beginners. And if we do it, we may have to do it for every function not just the ones in this PR to be consistent. I also don't see type hints to be very popular in major projects.

Any thoughts from others?

@xyluo25
Copy link
Author

xyluo25 commented Feb 15, 2024

@Ziqi-Li,

Thank you for your prompt response. I'm reaching out to see the next steps we should take under your suggestion. Do you recommend reverting some commits, or should we proceed differently?

I'd also like to provide some context regarding the changes I made. For items 2., 3. and 5., I adhered to the official PEP 8 standards, supplemented by insights from the book Clean Python: Elegant Coding in Python by O'Reilly, to enhance our coding practices. Regarding item 1. and your earlier comment, I wanted to delve into the discussion found in the Reddit question: "Why optional type hinting in Python is not that popular?". It's noteworthy that optional type hinting is recommended by the Python Software Foundation (PSF), with Guido van Rossum himself highly recommended and highlighting its benefits in "Guido van Rossum on types, speed, and the future of Python" and further explored by Luciano Ramalho in his presentation "typing.Protocol: type hints as Guido intended". With Python 3.8 embracing type hints, major projects like Dropbox are updating their codebases, as seen in "Our journey to type checking 4 million lines of Python".

Additionally, if you have the capacity to assist with issues #147, #148, #149, and #150, it would be greatly appreciated. These issues refer to errors and improperly defined variables in the code.

Thank you very much for your time and consideration.

Warm regards,
Xiangyong Roy Luo

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.

4 participants