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

Standardize LULC column names to lucode #1729

Merged

Conversation

emilyanndavis
Copy link
Member

@emilyanndavis emilyanndavis commented Jan 7, 2025

Description

Fixes #1249. Specifically, Habitat Quality & Coastal Blue Carbon now use lucode as column names where these were previously called lulc, code, or lulc_code.

Checklist

  • Updated HISTORY.rst and link to any relevant issue (if these changes are user-facing)
  • Updated sample data and revision hash in Makefile
  • Updated test data and revision hash in Makefile
  • Updated the user's guide (see invest.users-guide PR #151)
  • Update user guide revision hash in Makefile (after the UG PR has been merged)
  • Tested the Workbench UI (no UI changes, but I did confirm the models run successfully via the workbench)

@emilyanndavis
Copy link
Member Author

The user guide builds are failing here because the existing user guide source has references to model specs that have been updated by this PR.

I did build the user guide locally against a local install of natcap.invest and confirmed that the HTML was generated successfully for all locales.

Copy link
Contributor

@davemfish davemfish left a comment

Choose a reason for hiding this comment

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

Thanks for doing this @emilyanndavis . These changes look good to me. The issue is marked with a minor release (3.15.0) milestone, which probably means we want to create either a feature branch or a release/3.15.0 branch to merge these changes into. I think it's targeted for a minor release because there are changes to input data requirements.

Since this isn't really a major feature that we need to keep working on, I think it makes sense to create a release/3.15.0 branch for the target of this PR. And I see you already did that on the UG PR. If we want to, we can merge that UG PR first, and then update the Makefile revision here.

For the test & sample data repos, it could have been a good idea to stash those updates in a release branch as well. Although it's not super likely we will want to make changes to those repos in the meantime so it might be okay. If we do have changes for a bugfix release in those repos, we can try to branch them off an earlier commit.

@emilyanndavis
Copy link
Member Author

Since this isn't really a major feature that we need to keep working on, I think it makes sense to create a release/3.15.0 branch for the target of this PR. And I see you already did that on the UG PR. If we want to, we can merge that UG PR first, and then update the Makefile revision here.

For the test & sample data repos, it could have been a good idea to stash those updates in a release branch as well. Although it's not super likely we will want to make changes to those repos in the meantime so it might be okay. If we do have changes for a bugfix release in those repos, we can try to branch them off an earlier commit.

Thank you! I had started to create release branches everywhere, but somehow I became convinced the user guide was the only place that was necessary. I'll update the target branch here, and I'll go ahead and revert the sample data & test data changes and merge those into release branches instead. Like you said, those are unlikely to cause problems, but I'd rather save future us the headache, just in case.

@emilyanndavis emilyanndavis changed the base branch from main to release/3.15.0 January 8, 2025 18:20
@emilyanndavis
Copy link
Member Author

emilyanndavis commented Jan 8, 2025

@davemfish Everything except the user guide should be ready now. As you suggested, let's pause this one until the user guide PR is merged, and then I'll update the revision hash here. Thanks!

Copy link
Contributor

@davemfish davemfish 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. Checks passing!

@davemfish davemfish merged commit be745a1 into natcap:release/3.15.0 Jan 8, 2025
29 checks passed
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.

Standardize field names in biophysical tables
2 participants