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

PVEngine.skip_step is not used #139

Open
kandersolar opened this issue Feb 25, 2022 · 1 comment
Open

PVEngine.skip_step is not used #139

kandersolar opened this issue Feb 25, 2022 · 1 comment

Comments

@kandersolar
Copy link
Contributor

PVEngine creates a mask indicating values that need not be considered in the calculation:

# Skip timesteps when:
# - solar zenith > 90, ie the sun is down
# - DNI or DHI is negative, which does not make sense
# - DNI and DHI are both zero
self.skip_step = (solar_zenith > 90) | (DNI < 0) | (DHI < 0) \
| ((DNI == 0) & (DHI == 0))

However, that mask is not actually used anywhere today. It used to be, but it looks like it stopped being used in #84 during the switch from iterative to vectorized code.

I think the underlying concept may still be useful even in the current vectorized approach, so I think it's probably worth seeing if we can start using it again somehow, otherwise it should just be deleted IMHO.

@anomam
Copy link
Contributor

anomam commented Feb 27, 2022

Thanks for catching this @kanderso-nrel !
Looks like I missed this part when updating things. I'm generally of the opinion of deleting dead code, and we can always find it again in the git history if necessary.

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

No branches or pull requests

2 participants