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

Add suitability raster assessment and site polygon assessment functionality #32

Merged
merged 60 commits into from
Oct 11, 2024

Conversation

BG-AIMS
Copy link
Contributor

@BG-AIMS BG-AIMS commented Oct 2, 2024

Add assess_region() and site_assess_region() functions to dynamically process raster and polygon suitability results with user inputs.

@BG-AIMS BG-AIMS self-assigned this Oct 2, 2024
reg_cache_dir = config["server_config"]["REGIONAL_CACHE_DIR"]
reg_cache_fn = joinpath(reg_cache_dir, "regional_cache.dat")
if isfile(reg_cache_fn)
@debug "Loading regional data cache from disk"
@eval const REGIONAL_DATA = deserialize($(reg_cache_fn))

reef_outline_path = joinpath(reef_data_path, "rrap_canonical_outlines.gpkg")
Copy link
Collaborator

Choose a reason for hiding this comment

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

One issue I have here is that we'd have to rename the canonical reefs file every time there's an update, won't we?

Why not just use the original filename?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original file name has the date/time of creation in it so I thought it was easier to rename it when it is uploaded to the input data teams folder. Or should i use find_latest_file() to find it and use the original name?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That would remove the need for us to manually do things, so I think use find_latest_file() (in fact, we should rename that to find_latest_outlines() or something more descriptive). It's an API-level utility too, so makes more sense to me for the function to live in the API rather than processing scripts.

Copy link
Collaborator

@ConnectedSystems ConnectedSystems left a comment

Choose a reason for hiding this comment

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

Looking pretty good! A bunch of small comments to be fixed up

Comment on lines 239 to 276
# Need reef outlines
gdf = reg_assess_data["reef_outlines"]
reef_outlines = buffer_simplify(gdf)
reef_outlines = polygon_to_lines.(reef_outlines)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are the reef outlines used for anything else?

Could this be done once when loading data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that would make the simplification and buffering more hardcoded, but that's probably going too in depth for stakeholders anyway?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, more hardcoded?

Moving this buffer_simplify / polygon_to_lines elsewhere so it only happens once is the same level of "hardcodedness" in my view.

Otherwise I've misunderstood your concern...

src/ReefGuideAPI.jl Show resolved Hide resolved
src/criteria_assessment/criteria.jl Outdated Show resolved Hide resolved
src/criteria_assessment/site_identification.jl Outdated Show resolved Hide resolved
Copy link
Collaborator

@ConnectedSystems ConnectedSystems left a comment

Choose a reason for hiding this comment

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

A few more comments that I just picked up

src/ReefGuideAPI.jl Show resolved Hide resolved
src/criteria_assessment/site_identification.jl Outdated Show resolved Hide resolved
@ConnectedSystems
Copy link
Collaborator

@arlowhite ready for a final review I think 😄

@ConnectedSystems ConnectedSystems linked an issue Oct 8, 2024 that may be closed by this pull request
Copy link
Contributor

@arlowhite arlowhite left a comment

Choose a reason for hiding this comment

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

looks good, though there's one x/y lon/lat mismatch that may be a bug, see comment "Bug??"

src/ReefGuideAPI.jl Outdated Show resolved Hide resolved
src/criteria_assessment/site_identification.jl Outdated Show resolved Hide resolved
src/criteria_assessment/site_identification.jl Outdated Show resolved Hide resolved
src/criteria_assessment/site_identification.jl Outdated Show resolved Hide resolved
src/criteria_assessment/site_identification.jl Outdated Show resolved Hide resolved
src/criteria_assessment/site_identification.jl Outdated Show resolved Hide resolved
src/criteria_assessment/tiles.jl Outdated Show resolved Hide resolved
src/site_assessment/best_fit_polygons.jl Show resolved Hide resolved
src/site_assessment/common_functions.jl Show resolved Hide resolved
BG-AIMS and others added 6 commits October 10, 2024 17:09
Previous implementation used the same values based on x_dist and latitude. I think this only works if the location sizes are equal in both x and y distance. In 99% of cases this will be true, but protecting against the rare case it is not.
@ConnectedSystems
Copy link
Collaborator

I think this is all good to go @arlowhite

@ConnectedSystems ConnectedSystems merged commit 39637e8 into main Oct 11, 2024
2 checks passed
@ConnectedSystems ConnectedSystems deleted the site-assessment branch October 11, 2024 03:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants