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

NSI utils for obtaining NSI data #95

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

ywkim312
Copy link
Member

@ywkim312 ywkim312 commented Jan 8, 2025

please test it by running test_nsi.py in tests/pyincore_data folder

@ywkim312 ywkim312 linked an issue Jan 8, 2025 that may be closed by this pull request
@ywkim312 ywkim312 self-assigned this Jan 8, 2025
@ywkim312 ywkim312 linked an issue Jan 8, 2025 that may be closed by this pull request
@ywkim312 ywkim312 marked this pull request as ready for review January 14, 2025 16:01
@navarroc
Copy link
Member

Should nsiutil be inside the "utils" folder?

@navarroc
Copy link
Member

I see censusutil is also at the top level, which is kind of odd since there is a utils folder. At some point we probably need to think about the structure of pyincore-data. Something like:
pyincore_data / utils / nsiutil
pyincore_data / nsi / nsiparser.py (which calls nsiutil maybe)

or it could be pyincore_data/nsiparser.py

@ywkim312
Copy link
Member Author

Should nsiutil be inside the "utils" folder?

That is a good question. I had a hard time naming it, and there was a censusutil in the same place, so I named it that way. Do you have any other idea for the name of this nsiutil? I think that this file is better positioned in its current location, but maybe the name should be changed. If you propose a different name, I will update the code accordingly.

@ywkim312
Copy link
Member Author

ywkim312 commented Jan 14, 2025

I see censusutil is also at the top level, which is kind of odd since there is a utils folder. At some point we probably need to think about the structure of pyincore-data. Something like: pyincore_data / utils / nsiutil pyincore_data / nsi / nsiparser.py (which calls nsiutil maybe)

or it could be pyincore_data/nsiparser.py

NSI parser sounds good. I will update using this name. Do you have other idea for censusutil as well? Anyway, I will only focus on NSI part for this PR and if we need to rename censusutil I will create another issue since it also is related to sample notebook as well.

Copy link
Member

@longshuicy longshuicy left a comment

Choose a reason for hiding this comment

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

One thing I noticed is there are projections crs='EPSG:4326' coded in various functions. Wondering if we can put this into a variable in a central place and refer to that in the code.

pyincore_data/nsiutil.py Outdated Show resolved Hide resolved
pyincore_data/nsiutil.py Outdated Show resolved Hide resolved
pyincore_data/utils/datautil.py Outdated Show resolved Hide resolved
pyincore_data/utils/datautil.py Show resolved Hide resolved
pyincore_data/utils/datautil.py Outdated Show resolved Hide resolved
pyincore_data/utils/datautil.py Outdated Show resolved Hide resolved
pyincore_data/utils/datautil.py Show resolved Hide resolved
pyincore_data/utils/datautil.py Outdated Show resolved Hide resolved
pyincore_data/utils/datautil.py Outdated Show resolved Hide resolved
@ywkim312
Copy link
Member Author

One thing I noticed is there are projections crs='EPSG:4326' coded in various functions. Wondering if we can put this into a variable in a central place and refer to that in the code.

This probably needs to be an independant issue after this get merged. At this moment, IN-CORE only uses 4326 but I was awaring of this issue and was thinking of adding the way of handling this but as you probably know, dealing with crs is much more complicated, only making it as variable might caused some other failure

@navarroc
Copy link
Member

The test folder does not need an __init__.py file. If you remove them from the "tests" folder and the "pyincore_data" folder everything should run fine. It looks like the top level one was added but not needed at some point and wasn't caught.

@ywkim312
Copy link
Member Author

The test folder does not need an __init__.py file. If you remove them from the "tests" folder and the "pyincore_data" folder everything should run fine. It looks like the top level one was added but not needed at some point and wasn't caught.

Got it so it is either to have the perfect init or just remove all in the tests. I updated the init.py in my previous push and it worked and this time I removed all of them in the tests folder and it worked. I will keep it as not having them in tests folder

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.

Create a method for providing the county fips information NSI data API to IN-CORE building inventory dataset
3 participants