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

updated imports in the solarposition.py file and applied ruff linter+formatter to the whole directory tree #2338

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

PhilBrk8
Copy link
Contributor

No description provided.

@PhilBrk8 PhilBrk8 changed the title updated imports updated imports in the solarposition.py file and applied ruff linter+formatter to the whole directory tree Dec 22, 2024
@cwhanse
Copy link
Member

cwhanse commented Dec 22, 2024

@PhilBrk8 in my view, library-wide reformatting should be a deliberate decision connected with a set of linting rules, and tied to a major release, i.e., whenever we decide to move to v1.0. Although the edits here are improvements, I am discouraged from accepting them without already having discussed the linter and rules in the CI and having concurrence from maintainers about those rules.

Can you explain the motivation to update just the solarposition.py file? It appears that you started there then branched out to the full directory tree.

@echedey-ls
Copy link
Contributor

Wow. These are some pretty big changes @PhilBrk8 !! I was halfway redacting when @cwhanse weighed in, but his comment sums up what I was about to say.

Just to clarify, many times some specific style has been adopted to make equations more readable (in general some parts of the code) so that also needs to be taken into account by using comment macros to turn off the formatter.

The imp package removal seems good to me, and narrowing the ImportError to ModuleNotFoundError, just to clean up some legacy code and avoid the highly-unlikely problems derived from the imports.

In any case, and only if you are motivated to do so, I encourage you to address any of the issues tagged as "good first issue", they really make for good improvements in this repo!! Feel free to reach out if you need any assistance 😉

@PhilBrk8
Copy link
Contributor Author

PhilBrk8 commented Dec 23, 2024

Hi @cwhanse & @echedey-ls
quite a long time ago @kandersolar told me he would generally be up for it if I want to make it a project for myself to bring pvlib up to a newer / more modern python version.

I initially just wanted to implement type hints into the functions, but then saw some other things along the way.
And I started with the solarposition.py because I have used the tutorial notebook for generating a polar&carthesian sunpath diagram previously for my hometown and was already familiar with it.
So I used the notebook to check if anything breaks by implementing type hints.
(Imported pvlib in development mode and re-run the sunpath calculation a few times)

Regarding the formatting:
Basically just because I have set formatOnSave=true in VS-Code.

I naively thought the PR is only applied for the commits up to the time of the PR-creation, but when I realized the newer ones are included automatically, I changed the PR-Name and left the commits for the whole tree in, to see what you think of it.


But yes, reviewing / refactoring / modernizing all files is way out of scope for it to be done this way.

I then thought initially I just do everything on my fork and keep it there, because I got curious about a modern & polars version of pvlib and thought it might be interesting to benchmark a "modern"-pvlib against the original at some point.
But if chenges turn out to be useful improvements, then this is also not the way it should be done I guess, but with a more thought through branching strategy.


So then I came up with the following idea / proposal:
Are you generally up to create a "development" / "modern" branch in the original repo?
I would then keep the changes in respective brances like:

  • feature/reformatting
  • feature/type_hints
  • feature/polars
  • feature/refactor
    And not merge them into main but into the "modern" branch. For now it seems like it could be possible to not break anything, but I've only really looked into solarposition.py and there might be breaking changes in other places.

Or should I rather create a discussion for that?

@kandersolar
Copy link
Member

quite a long time ago @kandersolar told me he would generally be up for it if I want to make it a project for myself to bring pvlib up to a newer / more modern python version.

I'm not sure what "newer / more modern" means here -- python itself (e.g. python 3.13?), or dependencies like polars? If the latter, I think there was likely some misunderstanding. For example I thought the proposal here was more related to functionality/models than python versions: #1997 (comment)

In any case, it is best to open an issue proposing the change before putting in the work for a PR. Sometimes there is a better way to accomplish the goal, or maybe the change is not desirable for reasons that could be explained in the issue. Having that discussion first helps prevent wasted effort if the PR has to be changed or closed after the discussion.

So then I came up with the following idea / proposal:

Several of these topics already have discussions, e.g. #1926 and #2062. If you want to pursue these ideas, I think the first step is to get consensus that the idea is beneficial for pvlib. None of the mentioned ideas (bulk reformatting, type hints, polars) have achieved that consensus yet. So engaging in those discussions with new and persuasive arguments in favor of the ideas is the next step. Only once the community is convinced does it make sense to talk about how specifically it should be implemented, e.g. branching strategy and such.

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