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

Urban Nature Access - Split radii by population groups #1150

Conversation

phargogh
Copy link
Member

@phargogh phargogh commented Jan 21, 2023

This PR implements the final optional module in the Urban Nature Access model.

RE: #722

Checklist

  • Updated HISTORY.rst and link to any relevant issue (if these changes are user-facing)
  • Updated the user's guide (if needed)
  • Tested the affected models' UIs (if relevant)

…722-urban-nature-access-model-split-pop-and-radii
…722-urban-nature-access-model-split-pop-and-radii
…argogh/invest into feature/722-urban-nature-access-model
…argogh/invest into feature/722-urban-nature-access-model-split-pop-and-radii
Radius mode will control which of the optional modules will operate.

RE:natcap#722
This is for split population,radii, specifically.  RE:natcap#722
Refactor improves readability and eliminates the possibility of a NaN
getting into the search radii.

RE:natcap#722
This way, the radius definition mode will be able to produce exactly the
vector needed with whatever fields and/or calculations are needed for
the model  RE:natcap#722
The search radius modes with per-greenspace radii and uniform radius
define a few rasters in exactly the same way, so this moves those steps
into a shared place in the task execution tree so they only need to be
defined once. RE:natcap#722
Copy link
Member

@dcdenu4 dcdenu4 left a comment

Choose a reason for hiding this comment

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

Hey @phargogh, just getting around to taking a look at the testing. Just a few comments below.

  • Looks like pandas.testing is not used
  • I noticed a few tests for density and dichotomous kernels but not exponential, gaussian, or power. Was just wondering if all the kernels should have a test (for consistency) or if there was a reason for singling those ones out.

That's it!

@phargogh
Copy link
Member Author

phargogh commented Feb 3, 2023

Thanks @dcdenu4 ! I added a couple changes to:

  • Add tests for the rest of the kernel functions
  • Include a link to the draft UG chapter (some of the model team were asking about it)

So, I think I've responded to everything so far! But there are still a couple things that have come out of this review so far that I think would benefit from their own PRs:

  1. Implement conditional requirement at a more granular level than just the top-level keys: Implement conditional requirement for required keys anywhere in the args spec #1165
  2. Define a 'safe' layer name in the target layer: Define a 'safe' layer name in Urban Nature Access #1164
  3. There's still a need to define the power function's beta parameter for each possible search kernel, which means adding another column to the various tables where search radii can be defined.
  4. A reviewer mentioned some more sensible naming conventions for some of the outputs, which I believe are also reflected in the paper that ultimately describes this model.

So aside from these things, I'm happy to have this merged into the feature branch if you are!

@phargogh phargogh requested a review from dcdenu4 February 7, 2023 23:07
@dcdenu4 and I decided that because the power function is incompletely
specified in the design document and it'll take a little work to
implement properly, and also because the core model parts are working
even without the power kernel.  The power kernel can, if needed, be
added in to the model later on, like in a bugfix release.

RE:natcap#722
Copy link
Member

@dcdenu4 dcdenu4 left a comment

Choose a reason for hiding this comment

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

Thanks @phargogh, I am happy to merge this into the feature branch and tackle the remaining issues in separate PRs.

@dcdenu4 dcdenu4 merged commit db677b2 into natcap:feature/urban-nature-access Feb 10, 2023
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.

4 participants