Open xyluo25 opened 9 months ago
@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 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
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.
Thank you @martinfleis. I will follow your comments by opening issues in the repository to disscuse possible changes first.
@xyluo25 I have reviewed each line of the change. Just to help summarize what you have done:
assertAlmostEqual()
in the unittestsWhat 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?
@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
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