Skip to content

Add site and species statistics to CCA#92

Merged
grovduck merged 4 commits intomainfrom
add-cca-properties
May 2, 2025
Merged

Add site and species statistics to CCA#92
grovduck merged 4 commits intomainfrom
add-cca-properties

Conversation

@grovduck
Copy link
Copy Markdown
Member

@grovduck grovduck commented May 2, 2025

This PR adds four additional properties to sknnr.transformers._cca.ConstrainedOrdination. The properties are standard output used in pynnmap ordination files even though they aren't directly used in the CCA transformation. Rather than calculate these independently in pynnmap, it's more convenient to expose these as properties in sknnr even if they are rarely used.

The properties added are:

  • species_weights (total abundance for each species)
  • species_n2 (Hill's N2 diversity for species across all sites)
  • site_weights (total abundance for each site)
  • site_n2 (Hill's N2 diversity for sites across all species)

Because pynnmap is reliant upon this change happening (lemma-osu/pynnmap#14), I'm bumping the version to 0.1.0a3 as well.

@grovduck grovduck self-assigned this May 2, 2025
@grovduck grovduck added the enhancement New feature or request label May 2, 2025
@grovduck
Copy link
Copy Markdown
Member Author

grovduck commented May 2, 2025

@aazuspan, I took a bit of a tangent this week to get sknnr working in pynnmap and this issue came up during the initial PR for that work. Even though the CCA class is definitely still a bit of a mess (e.g. #60), I think these new properties are commonly used in CCA and worth exposing. Hopefully, this should be a quick review, but I intend on bumping the version as well.

@grovduck grovduck requested review from aazuspan and Copilot May 2, 2025 18:59
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds four new properties to ConstrainedOrdination to expose species and site statistics (abundance and Hill's N2 diversity), simplifying their use in pynnmap.

  • Added species_weights and species_n2 properties for species-level metrics.
  • Added site_weights and site_n2 properties for site-level metrics.
  • Bumped the version in about.py from 0.1.0a2 to 0.1.0a3.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/sknnr/transformers/_cca.py Added properties for species and site statistics to the CCA transformer.
src/sknnr/about.py Updated the version to support pynnmap requirements.

Comment thread src/sknnr/transformers/_cca.py
Comment thread src/sknnr/transformers/_cca.py
Copy link
Copy Markdown
Contributor

@aazuspan aazuspan left a comment

Choose a reason for hiding this comment

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

@grovduck, that's very exciting that sknnr's starting to get integrated into pynnmap!

I disagreed with the Copilot reviews about zero division errors since I think you've already covered that contingency, but we could leave those in for redundant safety if you prefer. Other than that, LGTM.

Do you want to make the PyPI release once this is merged?

@grovduck
Copy link
Copy Markdown
Member Author

grovduck commented May 2, 2025

I disagreed with the Copilot reviews about zero division errors since I think you've already covered that contingency, but we could leave those in for redundant safety if you prefer.

Good catch. Yep, you're absolutely right. I'm a bit too reliant on AI code changes.

Do you want to make the PyPI release once this is merged?

I will certainly give it a shot! Fingers crossed I don't screw anything up.

@grovduck grovduck merged commit 051f358 into main May 2, 2025
11 checks passed
@grovduck grovduck deleted the add-cca-properties branch May 2, 2025 21:09
@aazuspan
Copy link
Copy Markdown
Contributor

aazuspan commented May 2, 2025

Let me know if you run into issues!

FYI I'm trying to set up a workflow on another package (aazuspan/eerepr#65) to automate PyPI releases. If that works well there, I can get it set up for sknnr and sklearn-raster to save us some trouble in the future.

@grovduck
Copy link
Copy Markdown
Member Author

grovduck commented May 2, 2025

@aazuspan, I got it released on PyPI, but curious how you typically publish.

It sounds like there are a bunch of different ways including:

  1. storing the API key in a global hatch setting
  2. storing the API in the project directory in a .env file
  3. storing it as a session-based environmental variable.

I created the API key to just have upload permission on sknnr, so I didn't go for the first option. The second option is to do this in .env:

HATCH_INDEX_AUTH__PYPI=pypi-xxxxxx

but then it looks like you would need to modify the pyproject.toml file to find it, via

[tool.hatch.envs.default]
dotenv = ".env"

but that would affect both of us, so that didn't seem like a great solution.

Finally I tried to set it temporarily in my shell:

set HATCH_INDEX_AUTH__PYPI=pypi-xxxxxx

and then run hatch publish but it still prompted me for username (set as __token__) and the API key. I'm curious if you have a different path?

FYI I'm trying to set up a workflow on another package (aazuspan/eerepr#65) to automate PyPI releases. If that works well there, I can get it set up for sknnr and sklearn-raster to save us some trouble in the future.

EDIT: Sorry, I totally missed this response before writing this. Automating this workflow sounds cool. I see the section:

permissions:
            id-token: write  # IMPORTANT: mandatory for trusted publishing

Is that all it takes??

@aazuspan
Copy link
Copy Markdown
Contributor

aazuspan commented May 2, 2025

I'm curious if you have a different path?

No, I ran into the same complications you did and have just been using hatch publish -u __token__ -a APIKEY with a project-scoped API key like you mentioned. It seems reasonably secure since you don't have to store tokens, but it is a nuisance.

I think the workflow is definitely a better approach since it removes the token entirely and delegates authentication to Github - the only way to make a release is to have authority to push tags to the repository.

Is that all it takes??

There's also some setup required on the PyPI side to add the Github repository as a "Trusted publisher", but once that's done the workflow seemed to run perfectly. I'll make a PR to add that workflow and update the contributing guide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants