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

Update colorbar tick names in sunpath gallery example #2097

Merged
merged 6 commits into from
Jun 21, 2024

Conversation

AdamRJensen
Copy link
Member

@AdamRJensen AdamRJensen commented Jun 20, 2024

@AdamRJensen AdamRJensen marked this pull request as ready for review June 20, 2024 17:32
@AdamRJensen
Copy link
Member Author

@echedey-ls, @IoannisSifnaios, @RDaxini

Hi GSoC students
This PR is rather simple, but could be a good exercise for you to review

@echedey-ls
Copy link
Contributor

@echedey-ls
Copy link
Contributor

echedey-ls commented Jun 20, 2024

Let's add @PhilBrk8 to the whatsnew file:

* :ghuser:`PhilBrk8`

EDIT: LGTM the code from a quick glance

Copy link
Contributor

@IoannisSifnaios IoannisSifnaios left a comment

Choose a reason for hiding this comment

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

Looks great! Just a few ideas from my side

docs/examples/solar-position/plot_sunpath_diagrams.py Outdated Show resolved Hide resolved
@@ -128,8 +142,10 @@
label = date.strftime('%Y-%m-%d')
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
label = date.strftime('%Y-%m-%d')
label = date.strftime('%b %d$^{st}$')

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest you also do that in the previous figure

Copy link
Contributor

Choose a reason for hiding this comment

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

I find it very annoying that I can't comment on lines that have not been changed... I guess this is a GitHub thing though! Anyway, maybe also modify the text to say that ".... The solid colored lines show the...."

RDaxini

This comment was marked as duplicate.

Copy link
Contributor

@RDaxini RDaxini left a comment

Choose a reason for hiding this comment

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

Thanks for the notification @AdamRJensen .
Mostly looks fine to me but two things I noticed:

  1. Could we switch to 24hr format for the times in the docs? As opposed to writing AM/PM. Not sure if there is a preference for pvlib docs but I think it would be a more standard way to write the times, easily understood, and also be consistent with the times 06--19 on the figure already.
  2. Is it possible them to align the times on the figure so that they are all above the orange 2019-06-21 line, as 19 is?

@AdamRJensen
Copy link
Member Author

@kandersolar I've addressed @RDaxini's review as well so this PR should be ready to merge. Might as well get it out of the way with the release of 0.11.0.

@kandersolar kandersolar modified the milestones: v0.11.1, 0.11.0 Jun 21, 2024
@kandersolar kandersolar merged commit 3d778ea into pvlib:main Jun 21, 2024
24 of 25 checks passed
@PhilBrk8
Copy link
Contributor

Let's add @PhilBrk8 to the whatsnew file:

* :ghuser:`PhilBrk8`

EDIT: LGTM the code from a quick glance

The Whatsnew-file is what exactly?
I get notified of every change?

  • Would be down for it.
    ...even if life gets in the way a lot, I still plan on contributing more to Open Source and picked out this library to do so -

@echedey-ls
Copy link
Contributor

Hey @PhilBrk8 !

The Whatsnew-file is what exactly?

It is the changelog, a set of files that summarize the changes made to each one of the past releases and the next one. They are the source file(s) to build the following page: https://pvlib-python.readthedocs.io/en/stable/whatsnew.html

I get notified of every change?

You won't. We mention you in the contributors section. It's a way of encouraging and giving credit to contributors. You can find yourself listed on the previous link. Even though your PR wasn't merged, the original idea is still yours.

Feel free to contribute whenever you can and want, you will be welcomed here!

@AdamRJensen AdamRJensen deleted the sun_path_diagram_update branch June 24, 2024 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants