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

Easier State Entries #12

Closed
wants to merge 2 commits into from
Closed

Easier State Entries #12

wants to merge 2 commits into from

Conversation

AMDeitsch
Copy link

@AMDeitsch AMDeitsch commented Dec 1, 2023

I added a list of variables that can be called for stateFIPS.

For example, the following three lines of code will all return the same dataset for the District of Columbia:

aqs.bystate.sampledata(parameter = param, bdate = bdate, edate = edate , stateFIPS = aqs.DC)

aqs.bystate.sampledata(parameter = param, bdate = bdate, edate = edate , stateFIPS = aqs.DistrictOfColumbia)

aqs.bystate.sampledata(parameter = param, bdate = bdate, edate = edate , stateFIPS = 11)

Notes:

  1. Variables are case-sensitive.
  2. Spaces are omitted, not replaced with '_' (e.g., aqs.DistrictOfColumbia).
  3. "Country Of Mexico" is replaced with "Mexico" (e.g., aqs.Mexico).
  4. aqs.* prefix is required to call the variables.
  5. Abbreviation used for Canada is aqs.CAN to prevent conflict with California's aqs.CA.

@mccroweyclinton-EPA
Copy link
Collaborator

mccroweyclinton-EPA commented Dec 12, 2023

@AMDeitsch, thank you for your submission of code to the pyaqsapi repository. I am sorry it is taking so long to look through your pull request. To be honest, since pyaqspi is such a simple library. I had not thought this package would get a lot of attention, let alone people wanting to contribute to it. Being the sole maintainer of this repository I had planned on not accepting outside code contributions since I am unsure if I will have the bandwidth to review and test out each and every line of contributed code that is submitted with the exception of urgent issues like security vulnerabilities or unstable code.

That being said, before I make a final decision on accepting your code can you please tell me why you think this addition of code in necissarry? From glancing over your code it appears as if you are defining variables for state codes abbreviations. So the user can, for example, pass in the state abbreviation for aqs.DC for the District of Columbia, using your code, instead of directly using the stateFIPS '11' . This same result can be achieved by using the listfunctions.aqs_states() function and allowing the user to define their own variables for states that they wish to use based upon the values returned from listfunctions.aqs_states(), then passing that user defined variable to the function if they wish. I do understand how passing in a state abbriavation will make the code easier to read, but couldn't the user do that themself? Please give me your thoughts on why your contributed code should be accepted as this as this will help me to better consider if this contribution should be accepted. One goal I have is to try to maintain as much feature parity between pyaqsapi and it's related project RAQSAPI. So if I make this change here I will need to also implement the same feature there. I am going to discuss your contribution with my colleagues to determine if I should accept this. It will be a lot easier for us to make a determination if you can give a brief explanation as to why you feel this is necissarry.

Also it seems that if your goal is to retrieve data from Canada; so wouldn't it be easier for the user in that case to define their own variable, for example CC as opposed to typing aqs.CAN every time that user wishes to retrieve air monitoring data for Canada?

Again thank you for your interest in pyaqsapi and for your contribution to the project.

@AMDeitsch
Copy link
Author

AMDeitsch commented Dec 12, 2023

@AMDeitsch, thank you for your submission of code to the pyaqsapi repository. I am sorry it is taking so long to look through your pull request. To be honest, since pyaqspi is such a simple library. I had not thought this package would get a lot of attention, let alone people wanting to contribute to it. Being the sole maintainer of this repository I had planned on not accepting outside code contributions since I am unsure if I will have the bandwidth to review and test out each and every line of contributed code that is submitted with the exception of urgent issues like security vulnerabilities or unstable code.

That being said, before I make a final decision on accepting your code can you please tell me why you think this addition of code in necissarry? From glancing over your code it appears as if you are defining variables for state codes abbreviations. So the user can, for example, pass in the state abbreviation for aqs.DC for the District of Columbia, using your code, instead of directly using the stateFIPS '11' . This same result can be achieved by using the listfunctions.aqs_states() function and allowing the user to define their own variables for states that they wish to use based upon the values returned from listfunctions.aqs_states(), then passing that user defined variable to the function if they wish. I do understand how passing in a state abbriavation will make the code easier to read, but couldn't the user do that themself? Please give me your thoughts on why your contributed code should be accepted as this as this will help me to better consider if this contribution should be accepted. One goal I have is to try to maintain as much feature parity between pyaqsapi and it's related project RAQSAPI. So if I make this change here I will need to also implement the same feature there. I am going to discuss your contribution with my colleagues to determine if I should accept this. It will be a lot easier for us to make a determination if you can give a brief explanation as to why you feel this is necissarry.

Also it seems that if your goal is to retrieve data from Canada; so wouldn't it be easier for the user in that case to define their own variable, for example CC as opposed to typing aqs.CAN every time that user wishes to retrieve air monitoring data for Canada?

Again thank you for your interest in pyaqsapi and for your contribution to the project.

@mccroweyclinton-EPA, Thanks for your response. I understand the situation on reviewing contributed code and the need to match any changes with the sister package for R. As you said, the user could easily pull up a list of the codes related with aqs.aqs_states() and this is not, by any means, a critical change. However, the existence of these definitions makes things a little simpler.

Now, responsibilities can be simplified by typing either the name or abbreviation of the value rather than taking on the extra steps to find it in the list and define it as a variable. Pulling up a list as aqs.aqs_states(), finding the right number, and then defining NY = 36 before pulling data bystate, while relatively simple, is much simpler if you can just use NewYork or NY. It is not absolutely necessary, but it does make things a bit more streamlined.

On the note of CC vs aqs.CAN, Canada is the only state that doesn't use a numeric value for its input code (and is currently under review in issue #11 ). While other states are abbreviated with two letters (e.g., Mexico as MX; "aqs.MX = 80"), I was keeping the input value for Canada separate from its abbreviation to avoid confusion, so it was not "aqs.CC= CC" Also, the user would only know the code for Canada is 'CC' if they were to go through the steps above to call the list of state codes. In general, the abbreviation for Canada would be CA but that would conflict with the abbreviation for California.

-- Adam

@mccroweyclinton-EPA
Copy link
Collaborator

Thanks for your submission, we may revisit this in the future to merge this feature in.

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

Successfully merging this pull request may close these issues.

3 participants