Skip to content

improve speed of Location.get_airmass #528

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 3 commits into from
Aug 14, 2018
Merged

Conversation

wholmgren
Copy link
Member

@wholmgren wholmgren commented Aug 13, 2018

  • Closes slow performance when assigning to empty DataFrame in Location.get_airmass and ModelChain.prepare_inputs #502
  • I am familiar with the contributing guidelines.
  • Fully tested. Added and/or modified tests to ensure correct behavior for all reasonable inputs. Tests (usually) must pass on the TravisCI and Appveyor testing services.
  • Updates entries to docs/sphinx/source/api.rst for API changes.
  • Adds description and name entries in the appropriate docs/sphinx/source/whatsnew file for all changes.
  • Code quality and style is sufficient. Passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • New code is fully documented. Includes sphinx/numpydoc compliant docstrings and comments in the code where necessary.
  • Pull request is nearly complete and ready for detailed review.

@wholmgren wholmgren added this to the 0.6.0 milestone Aug 13, 2018
Copy link
Member

@mikofski mikofski left a comment

Choose a reason for hiding this comment

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

I think an even faster way to do this would be:

airmass = pd.DataFrame(
    {'airmass_relative': airmass_relative,
     'airmass_absolute': airmass_absolute},
    index=solar_position.index)

Nope I was wrong, it's already faster the way it is, and it keeps the correct order for all pythons b/c py2 dictionaries are unordered

@wholmgren
Copy link
Member Author

@mikofski see the linked issue for timings. I slightly prefer this approach for style reasons. Happy to change if most people prefer the other approach.

Copy link
Member

@mikofski mikofski left a comment

Choose a reason for hiding this comment

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

Okay I was wrong, I like it the way it is now. Thanks Will!

@wholmgren wholmgren merged commit 46059b5 into pvlib:master Aug 14, 2018
@wholmgren wholmgren deleted the speedups branch August 14, 2018 23:46
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