-
Notifications
You must be signed in to change notification settings - Fork 1
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
base: develop
Are you sure you want to change the base?
NSI utils for obtaining NSI data #95
Conversation
Should nsiutil be inside the "utils" folder? |
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: or it could be pyincore_data/nsiparser.py |
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. |
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. |
There was a problem hiding this 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.
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 |
The test folder does not need an |
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 |
please test it by running test_nsi.py in tests/pyincore_data folder