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.

Sign up for GitHub

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

Jump to bottom

Why does SDR still require the ws_id field in the watershed input? #1201

Closed
newtpatrol opened this issue Feb 27, 2023 · 1 comment · Fixed by #1204
Closed

Why does SDR still require the ws_id field in the watershed input? #1201

newtpatrol opened this issue Feb 27, 2023 · 1 comment · Fixed by #1204
Assignees
@phargogh
Labels
bug Something isn't working in progress This issue is actively being worked on

Comments

@newtpatrol
Copy link

A user brought this up on the forum today, noting that SWY does not require that the watershed input have a ws_id field, but SDR does. I'm pretty sure that the only reason that SDR does is legacy from the days when we provided a valuation step, such that different watersheds had different valuation parameters assigned. But we don't do that any more, so is there any reason to require the ws_id field in SDR? Can we remove that requirement?
@phargogh
Copy link
Member

Looking at the SDR source code, I can confirm that the ws_id field is no longer required and is not used except to validate that the field exists. So no need to have it there! I'll pull it.

@phargogh phargogh self-assigned this Feb 27, 2023
@phargogh phargogh added bug Something isn't working in progress This issue is actively being worked on labels Feb 27, 2023
phargogh added a commit to phargogh/invest that referenced this issue Feb 27, 2023
@phargogh
Removing the ws_id field requirement in SDR.
ff83d6f 
This field is a relic from earlier versions of the model where we
provided an optional valuation step that associated valuation parameters
with the watersheds via the WS_ID field.  We removed the valuation
component, but forgot to remove the required WS_ID field.

RE:natcap#1201
@phargogh phargogh mentioned this issue Feb 27, 2023
Merged
3 tasks
phargogh added a commit to phargogh/invest that referenced this issue Feb 27, 2023
@phargogh
Fixing a missing field in the args spec.
0e9cc7a 
Also improving readability of tests.

RE:natcap#1201
phargogh added a commit to natcap/invest.users-guide that referenced this issue Feb 27, 2023
@phargogh
Removing the ws_id field.
a270a66 
See natcap/invest#1201
phargogh added a commit to phargogh/invest that referenced this issue Feb 27, 2023
@phargogh
Bumping UG repo rev to remove the ws_id requirement from SDR UG.
44877d3 
RE:natcap#1201
@phargogh phargogh mentioned this issue Mar 3, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@phargogh phargogh

Labels
bug Something isn't working in progress This issue is actively being worked on
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Removing the ws_id field requirement in SDR. phargogh/invest
2 participants
@phargogh @newtpatrol