Skip to content

fix modelchain poa tests #1052

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

Merged
merged 2 commits into from
Sep 8, 2020
Merged

fix modelchain poa tests #1052

merged 2 commits into from
Sep 8, 2020

Conversation

wholmgren
Copy link
Member

@wholmgren wholmgren commented Sep 7, 2020

  • Closes #xxxx
  • I am familiar with the contributing guidelines
  • Tests added
  • Updates entries to docs/sphinx/source/api.rst for API changes.
  • Adds description and name entries in the appropriate "what's new" file in 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`).
  • New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels and Milestone are assigned to the Pull Request and linked Issue.

As discussed here, test_prepare_inputs_from_poa didn't work when I ran the tests locally (as I expect from looking at the code) but the CI never failed (for reasons I still don't understand). This PR fixes that.

This PR also changes the modelchain.WEATHER_KEYS, POA_DATA_KEYS, TEMPERATURE_KEYS, and DATA_KEYS to tuples. The problem with using a set here is that it's unordered, so the _assign_weather and _assign_total_irrad create dataframes with column order that depends on the platform.

@wholmgren wholmgren added this to the 0.8.0 milestone Sep 7, 2020
@wholmgren
Copy link
Member Author

While we're at it, I wonder about the name POA_DATA_KEYS. Would POA_KEYS be more consistent with WEATHER_KEYS and TEMPERATURE_KEYS?

@wholmgren wholmgren mentioned this pull request Sep 7, 2020
@wholmgren wholmgren mentioned this pull request Sep 8, 2020
8 tasks
@wholmgren wholmgren requested a review from cwhanse September 8, 2020 16:48
Copy link
Member

@cwhanse cwhanse left a comment

Choose a reason for hiding this comment

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

Were you able to figure out why the test passed the CI with that bad logic?

@wholmgren
Copy link
Member Author

I was not able to figure that out, unfortunately. The fixtures are all function scoped either explicitly or by default, so I don't think they're the cause of the problem.

@wholmgren wholmgren merged commit bc3a9c8 into pvlib:master Sep 8, 2020
@wholmgren wholmgren deleted the poatest branch September 8, 2020 17:41
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.

2 participants