-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Spa.julian day microsecond fix #942
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
Spa.julian day microsecond fix #942
Conversation
… test in tests_spa to check for correct scaling
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.
Thanks @ericf900! One minor formatting comment below. It might also be a good idea to note in the whatsnew that this function is not used in the typical SPA solar position workflow so users shouldn't expect their old results to be wrong, but I'll leave it to others to decide if it's needed and if so what the wording should be.
@@ -60,6 +62,8 @@ Testing | |||
dependent iotools tests to repeat them on failure. (:pull:`919`) | |||
* Separate azure-pipelines.yml platform-specific tests to their own templates | |||
located in ``./ci/azure/``. (:pull:`926`) | |||
* Updated test_julian_day_dt in spa_test to check for correct microsecond | |||
scaling (:issue:`940') (:pull:`942`) |
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.
Accidentally used apostrophe here instead of backtick, check the formatting here: https://pvlib-python--942.org.readthedocs.build/en/942/whatsnew.html#testing
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.
Same for the entry in "bug fixes"
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.
no need to make a separate what's new entry for the test fix.
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.
Thanks Will and Kevin! I made those changes
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.
Thanks @ericf900!
@@ -60,6 +62,8 @@ Testing | |||
dependent iotools tests to repeat them on failure. (:pull:`919`) | |||
* Separate azure-pipelines.yml platform-specific tests to their own templates | |||
located in ``./ci/azure/``. (:pull:`926`) | |||
* Updated test_julian_day_dt in spa_test to check for correct microsecond | |||
scaling (:issue:`940') (:pull:`942`) |
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.
no need to make a separate what's new entry for the test fix.
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.
thanks @ericf900 !
docs/sphinx/source/whatsnew
for all changes. Includes link to the GitHub Issue with:issue:`num`
or this Pull Request with:pull:`num`
. Includes contributor name and/or GitHub username (link with:ghuser:`user`
).