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

Input parameter naming convention - time vs times #2231

Open
AdamRJensen opened this issue Sep 28, 2024 · 14 comments
Open

Input parameter naming convention - time vs times #2231

AdamRJensen opened this issue Sep 28, 2024 · 14 comments

Comments

@AdamRJensen
Copy link
Member

AdamRJensen commented Sep 28, 2024

Several pvlib functions take an index as one of the input parameters, however, it is inconsistent whether this parameter is called times or time.

I think we should document the preferred naming, add it to the symbols and variables list, and make an effort to standardize.

See a few examples below of TIME:

def get_solarposition(time, latitude, longitude,

def spa_c(time, latitude, longitude, pressure=101325, altitude=0,

def localize_to_utc(time, location):

Here's a few examples of TIMES:

def _get_sample_intervals(times, win_length):

def sun_rise_set_transit_spa(times, latitude, longitude, how='numpy',

def hour_angle(times, longitude, equation_of_time):

@RDaxini
Copy link
Contributor

RDaxini commented Sep 28, 2024

Related: #1253
In particular this comment (btw I have updated the table with time/times now thanks to this issue)

@adriesse
Copy link
Member

adriesse commented Oct 1, 2024

I vote for time.

@AdamRJensen
Copy link
Member Author

I personally prefer times 😋

@kandersolar
Copy link
Member

A good guiding principle may be to use plural words only when multiple values are necessary. For example, I think times is an appropriate name for the _get_sample_intervals(times, win_length) function, since it actually requires multiple timestamps. For other functions (e.g. hour_angle) that work fine with single timestamps, would we say that the function returns hour_angles, and do the rise/set functions return sunrises and sunsets? Here I think singular makes sense.

An argument against time in general is that it has the same name as a python standard library.

@cwhanse
Copy link
Member

cwhanse commented Oct 1, 2024

An argument against time in general is that it has the same name as a python standard library.

Good point. I overlooked this when agreeing with "time" with the singular/plural grammar in mind. I retract my vote; pvlib should not shadow any standard python names.

@adriesse
Copy link
Member

adriesse commented Oct 2, 2024

An argument against time in general is that it has the same name as a python standard library.

Good point. I overlooked this when agreeing with "time" with the singular/plural grammar in mind. I retract my vote; pvlib should not shadow any standard python names.

As the shadowing only occurs within the function, this doesn't seem so serious. It will not cause any problems for the user of the function.

@cwhanse
Copy link
Member

cwhanse commented Oct 2, 2024

As the shadowing only occurs within the function, this doesn't seem so serious. It will not cause any problems for the user of the function.

Yes, but we can't anticipate how a user structures their code.

@cwhanse cwhanse closed this as completed Oct 2, 2024
@cwhanse cwhanse reopened this Oct 2, 2024
@echedey-ls
Copy link
Contributor

As the shadowing only occurs within the function, this doesn't seem so serious. It will not cause any problems for the user of the function.

Yes, but we can't anticipate how a user structures their code.

I don't see an issue and fully agree with @adriesse on that it is not a problem. Can you provide an example of what you have in mind? AFAIK, this parameter will only override time identifier inside its namespace, the pvlib func.

@cwhanse
Copy link
Member

cwhanse commented Oct 2, 2024

I can't provide an example where code fails to run. And we've had time as a function parameter for years, so I yield my objection.

@echedey-ls
Copy link
Contributor

No problem.

@echedey-ls
Copy link
Contributor

I was about to edit this function:

def sun_rise_set_transit_spa(times, latitude, longitude, how='numpy',

in #2237 to add another example and test that all works flawlessly, but reading through its documentation I think the appropriate rename is date instead of times. From the docs (and code) of the internal function it uses:

def transit_sunrise_sunset(dates, lat, lon, delta_t, numthreads):

These "times" must be at 00:00. Funny, isn't it?

Feel free to weight in if there are other words to be considered that I've missed.

@adriesse
Copy link
Member

adriesse commented Oct 3, 2024

No opinion here for pvlib, but in my own work I standardized on time for many dates ago. I see date simply as low-resolution time, with the time-of-day unspecified.

@echedey-ls
Copy link
Contributor

I see date simply as low-resolution time, with the time-of-day unspecified.

That did convince me.

@echedey-ls
Copy link
Contributor

Shall transit_sunrise_sunset:

def transit_sunrise_sunset(dates, lat, lon, delta_t, numthreads):

be changed to time too? It requires the time part to be exactly at 00:00.

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

6 participants