Add project setup CLI script#75
Conversation
There was a problem hiding this comment.
Edit: apologies if this PR wasn't ready for review - I missed that it was still marked as a draft
Nice! This'll be sweet to get in the repo rather than needing that separate ArcGIS workflow.
I've left some comments with questions/potential suggestions, lmk if you want to chat more on anything
arpitjain099
left a comment
There was a problem hiding this comment.
Nice addition - having a setup script that handles directory creation + DEM clipping + site file in one step will save a lot of manual work.
A couple things I noticed:
-
The new
make_NMSIM_site_dir()andcreate_NMSIM_site_file()use Windows-style backslash paths (e.g.r"Input_Data\01_ELEVATION",r"Input_Data\05_SITES"). Since the codebase is moving toward cross-platform support, these could useos.path.join("Input_Data", "01_ELEVATION")etc. Same foroutput_baseinproject_setup.py. -
The
coords_to_utm()return type changes fromstrtotuple[str, int]here, which is a good change (callers often need the zone number). Worth noting that PR #84 also touchescoords_to_utm()callers, so there will be a merge conflict between the two. Happy to rebase #84 once this lands, or we can coordinate. -
Small typo in scripts/README.md: "Fundemental" should be "Fundamental".
-
Missing newline at end of
helpers.py.
|
Will plan to take another look a little later today!
…On Thu, Jul 9, 2026 at 12:37 PM D.Halyn Betchkal ***@***.***> wrote:
@dbetchkal <https://github.com/dbetchkal> requested your review on:
dbetchkal/NPS-ActiveSpace#75
<#75> Add project setup
CLI script.
—
Reply to this email directly, view it on GitHub
<#75?email_source=notifications&email_token=AFP7WUG42EQ3NEG7GY5SZ735D7C5BA5CNFSNUABQM5UWIORPF5TWS5BNNB2WEL2JONZXKZKFOZSW45CON52GSZTJMNQXI2LPNYXTENZXG42TIOJQHEYTTJTSMVQXG33OWBZGK5TJMV3V64TFOF2WK43UMVSKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#event-27775490919>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFP7WUEWSZZTISCSANSG7VD5D7C5BAVCNFSNUABFKJSXA33TNF2G64TZHMZTQOJXG42TKMRXHNEXG43VMU5TINZXGA4DSOBXGQZKC5QC>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/AFP7WUFIZ5QEIVXD2OO3QJL5D7C5BA5CNFSNUABQM5UWIORPF5TWS5BNNB2WEL2JONZXKZKFOZSW45CON52GSZTJMNQXI2LPNYXTENZXG42TIOJQHEYTTJTSMVQXG33OWBZGK5TJMV3V64TFOF2WK43UMVSKKZLWMVXHJKTGN5XXIZLSL5UW64Y>
and Android
<https://github.com/notifications/mobile/android/AFP7WUAE2SXTOIRYBBC4A4D5D7C5BA5CNFSNUABQM5UWIORPF5TWS5BNNB2WEL2JONZXKZKFOZSW45CON52GSZTJMNQXI2LPNYXTENZXG42TIOJQHEYTTJTSMVQXG33OWBZGK5TJMV3V64TFOF2WK43UMVSKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>.
Download it today!
You are receiving this because your review was requested.Message ID:
***@***.***>
|
elliott-ruebush
left a comment
There was a problem hiding this comment.
I missed commenting on this on first pass, but I was looking some more and wanted to raise the point that some of these setup functions are seemingly duplicated between here and ActiveSpaceGenerator https://github.com/dbetchkal/NPS-ActiveSpace/blob/main/nps_active_space/active_space/active_space_generator.py
| project_setup.py | ActiveSpaceGenerator |
|---|---|
| create_NMSIM_site_dir() | _make_dir_tree() |
| create_NMSIM_site_file() | _create_site_file() |
| create_gridfloat() | _create_dem_flt() |
| create_NMSIM_elevation_tif() | _mask_dem_file() (+ project_raster() in computation.py) |
So it seems that there's some repetition of function defintions across the two.
IMO the correct place for this to live might be project setup scripts, so maybe we move forward with merging this PR and then move the logic out of active space generator in a follow-up
|
Good catch on the duplication @elliott-ruebush. I ran into the same thing when reviewing - the DEM clipping and site file creation logic in ActiveSpaceGenerator does roughly the same work as what's here. Merging this first and then pulling the shared logic into one place sounds like the right order to me. Would keep this PR's scope clean and avoid a messy rebase. |
| help="Study area NE corner x y (WGS84). Example: -135.818994 58.706095") | ||
| args = parser.parse_args() | ||
|
|
||
| feet_to_meters = 0.3048 |
There was a problem hiding this comment.
No change needed on this PR.
One other minor [nit], I think that it's nice to define constants like these as all caps global vars at the top of the files. e.g. FEET_TO_METERS = 0.3048
I have a later change which will add a few general constants, so I'll take a quick pass at updating this then. Something like FEET_TO_METERs in particular feels like it could live somewhere central.
Closes #35. Closes #69. Intended to replace NMSIM_CreateBaseLayers.ipynb and to further escape the need for an
ArcGIS Prolicense.Flexible ingestion of the two contextual inputs towill add laterNPS-ActiveSpace(study area + listening location).NMSIMsite directory given inputs from config file.NMSIM.sit file to represent the listening location.