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
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
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.
Preserve dbetchkal's Inputs/Returns/Notes docstrings from PR #75 in the setup package, module preamble, NMSIM quirk comments, NLCD TODO, and CLI print messages. Match original function signatures for elevation/gridfloat. Co-authored-by: Cursor <cursoragent@cursor.com>
Use microphone.crs from get_deployment() when launching the ground-truthing app instead of passing the coords_to_utm() tuple. Co-authored-by: Cursor <cursoragent@cursor.com>
No active callers use the PR #75 create_NMSIM_* names; project_setup and tests already import the canonical symbols. Co-authored-by: Cursor <cursoragent@cursor.com>
…#102) * Fix ground-truthing CRS regression from PR #75. Use microphone.crs from get_deployment() when launching the ground-truthing app instead of passing the coords_to_utm() tuple. Co-authored-by: Cursor <cursoragent@cursor.com> * Extract project_setup logic into nps_active_space/setup. Move DEM clipping, GridFloat output, site directory creation, and setup_site orchestration into a library package. Thin project_setup.py to argparse and config only. Deduplicate site directory and .sit writing in ActiveSpaceGenerator. Add dem_elevation_units config and setup integration tests. Co-authored-by: Cursor <cursoragent@cursor.com> * Align .sit microphone coords to project UTM zone and add legacy migration. Write and read .sit easting/northing in the NMSIM project UTM zone (western study-area edge). Split site helpers into site_writer and site_decoder; fail clearly on legacy mic-local zone files with a project_setup rerun command. Add migrate_project_sites.py for in-place .sit rewrites and cross-zone regression tests. Co-authored-by: Cursor <cursoragent@cursor.com> * Remove unused backward-compat aliases from setup package. No active callers use the PR #75 create_NMSIM_* names; project_setup and tests already import the canonical symbols. Co-authored-by: Cursor <cursoragent@cursor.com> * Trim setup package exports and dedupe .sit parsing in get_deployment. Keep __init__ as a thin entry point for setup_site and NMSIM_DST_CRS; import site helpers from submodules. Use read_sit_file() instead of re-parsing .sit lines in helpers. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
3 participants
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.