Skip to content

deprecate tot_nitro slot#751

Draft
mslarae13 wants to merge 3 commits intomainfrom
680-term-total-nitrogen-concentration-more-general
Draft

deprecate tot_nitro slot#751
mslarae13 wants to merge 3 commits intomainfrom
680-term-total-nitrogen-concentration-more-general

Conversation

@mslarae13
Copy link
Collaborator

@mslarae13 mslarae13 commented Jan 13, 2024

Fixes #680

tot_nitro =
Total nitrogen concentration of water samples, calculated by: total nitrogen = total dissolved nitrogen + particulate nitrogen. Can also be measured without filtering, reported as nitrogen
domain_of:

  • HydrocarbonResourcesCores
  • HydrocarbonResourcesFluidsSwabs
  • WastewaterSludge
  • Water

tot_nitro_content =
Total nitrogen content of the sample
domain_of:

  • Agriculture
  • FoodFarmEnvironment
  • MicrobialMatBiofilm
  • Sediment
  • Soil

tot_nitro & tot_nitro_content are the same, with the exception that tot_nitro is water specific in its description

In this PR,

  • marked tot_nitro for deprecation
  • changed anywhere tot_nitro it used to now use tot_nitro_content
    • HydrocarbonResourcesFluidsSwabs
    • Water
    • WastewaterSludge
    • HydrocarbonResourcesCores

@cmungall
Copy link
Contributor

cmungall commented Apr 5, 2024

What is the MIxS policy on deprecation?

I would not delete this as you have done in the PR. Instead

  • mark as deprecated
  • and a replacement annotation to tot_nitro_content

Copy link
Contributor

@cmungall cmungall left a comment

Choose a reason for hiding this comment

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

deprecate instead of delete

@mslarae13 mslarae13 changed the title remove tot_nitro slot deprecate tot_nitro slot Jul 17, 2024
@mslarae13
Copy link
Collaborator Author

Discuss with CIG
IF there is need for HCR extensions to specify the tot_c content of the WATER vs the sample, we should update the description to emphasize that the water is NOT the same. And the tot_nitro_content should be used for the water extensions as in this case it is the sample.

In short
Is this slot describing a property of the SAMPLE or a some aspect of the ENVIRONMENT in which the sample was collected
See : #664 (comment)

@lschriml
Copy link
Member

lschriml commented Jul 18, 2024 via email

@mslarae13
Copy link
Collaborator Author

Thanks @lschriml

I realized that from a comment from @cmungall
With that, I agree we could probably close this PR and issue, but we should visit the included discussion to make sure we're providing clear text on when these data as metadata values are about the "sample or a some aspect of the environemnt in which the sample was collected"

@lschriml
Copy link
Member

lschriml commented Jul 19, 2024 via email

@turbomam
Copy link
Member

turbomam commented Jul 22, 2024

This is a great discussion about user requirements and GSC/MIxS policies, esp. with respect to the LinkML deprecation procedure that has been approved by the TWG.

For me the key issue here is the effort that goes into creating MIxS terms and Checklists/Extensions in support of specific users from specific domains, versus the ongoing effort that goes into managing MIxS as a whole. @mslarae13 has opened a lot of thoughtful issues and PRs about what appear to be redundant terms. Whether they are merged with changes or just closed, the general theme shouldn't be neglected.

Folklore-like reflection on the history of a term can be a great addition to a GitHub ticket like this, but it shouldn't be necessary. At a minimum the descriptions of the two terms (or comments?) should be updated with clear differentiating language. Moving forward, all of this knowledge should be captured in a rolling format that is easy to integrate with the LinkML representation of MIxS.

@lschriml
Copy link
Member

lschriml commented Jul 22, 2024 via email

@lschriml lschriml added the 5-TechnicalUpdates Updates to the LinkML representation or technical components of MIxS label Jan 7, 2025
@turbomam turbomam requested a review from Copilot March 11, 2026 00:30
Copy link
Contributor

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

Deprecates the water-specific total nitrogen slot and updates affected extensions to use tot_nitro_content (and associated method slot) instead, addressing #680’s concern about environment-specific term definitions in the MIxS schema.

Changes:

  • Removed the tot_nitro slot definition (MIXS:0000102) from the schema.
  • Replaced tot_nitro with tot_nitro_content (and added tot_nitro_cont_meth) in several extensions’ slot lists.
  • Added tot_nitro_cont_meth to additional extensions beyond those listed in the PR description.
Comments suppressed due to low confidence (2)

src/mixs/schema/mixs.yaml:14366

  • This change removes the tot_nitro slot (MIXS:0000102) entirely rather than deprecating it. If downstream users still provide tot_nitro in data, this becomes a breaking schema change; consider keeping the slot with deprecated: true (and a clear replacement pointer to tot_nitro_content) instead of deleting it outright.
      syntax: ^{scientific_float}( *- *{scientific_float})? *{text}$
      interpolated: true
      partial_match: true
  tot_nitro_cont_meth:
    description: Reference or method used in determining the total nitrogen
    title: total nitrogen content method
    examples:

src/mixs/schema/mixs.yaml:14380

  • After removing tot_nitro from the source schema, the committed generated artifacts (e.g., the Python datamodel) will be out of sync unless regenerated; the repo Makefile includes a gen-project step that outputs src/*/datamodel/*py. Please regenerate and commit the updated datamodel so it no longer exposes tot_nitro if the slot is truly being removed/deprecated.
  tot_nitro_cont_meth:
    description: Reference or method used in determining the total nitrogen
    title: total nitrogen content method
    examples:
      - value: https://currentprotocols.onlinelibrary.wiley.com/doi/abs/10.1002/0471142913.fab0102s00
    keywords:
      - content
      - method
      - nitrogen
      - total
    slot_uri: MIXS:0000338
    range: string
    pattern: ^^PMID:\d+$|^doi:10.\d{2,9}/.*$|^https?:\/\/(?:www\.)?[-a-zA-Z0-9@:%._\+~#=]{1,256}\.[a-zA-Z0-9()]{1,6}\b(?:[-a-zA-Z0-9()@:%_\+.~#?&\/=]*)$$
    structured_pattern:
      syntax: ^{PMID}|{DOI}|{URL}$
      interpolated: true
      partial_match: true
  tot_nitro_content:

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- tot_diss_nitro
- tot_inorg_nitro
- tot_nitro
- tot_nitro_content
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

In the Water extension, replacing tot_nitro (concentration) with tot_nitro_content (content) changes the meaning of the field and likely the expected units. If the intent is to make total nitrogen applicable beyond water, consider generalizing the existing concentration slot (MIXS:0000102) and deprecating/aliasing names, rather than switching to a different “content” slot.

Suggested change
- tot_nitro_content
- tot_nitro

Copilot uses AI. Check for mistakes.
Comment on lines 19364 to 19368
- temp
- tot_carb
- tot_nitro_content
- tot_nitro_cont_meth
- tot_org_carb
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

The PR description lists updates for HydrocarbonResources* / Water / WastewaterSludge, but this diff also adds tot_nitro_cont_meth to the MicrobialMatBiofilm extension. Please either update the PR description/scope to include this extension or revert this additional change if it’s unintended.

Copilot uses AI. Check for mistakes.
Comment on lines 19721 to 19727
@@ -19742,6 +19722,7 @@ classes:
- tot_carb
- tot_depth_water_col
- tot_nitro_content
- tot_nitro_cont_meth
- tot_org_carb
- turbidity
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

The PR description doesn’t mention the Sediment extension, but this change adds tot_nitro_cont_meth to its slot list. Please update the PR description/scope accordingly or drop this change if it’s out of scope for #680.

Copilot uses AI. Check for mistakes.
- nitrite
- ammonium
- tot_nitro
- tot_nitro_content
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

tot_nitro_content is defined as “Total nitrogen content of the sample” (mass per mass examples), whereas the removed tot_nitro term was explicitly a concentration term (mass per volume) with water-oriented preferred units. Swapping tot_nitrotot_nitro_content in this extension changes the semantics/expected units; consider instead generalizing tot_nitro’s description (removing the water-specific wording) or introducing a new generalized concentration term rather than reusing a “content” slot.

Suggested change
- tot_nitro_content
- tot_nitro

Copilot uses AI. Check for mistakes.
- nitrite
- ammonium
- tot_nitro
- tot_nitro_content
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

tot_nitro_content is a “content” slot (not a concentration slot), so replacing tot_nitro with tot_nitro_content here may change the intended measurement units/meaning for this extension. If the goal is only to remove water-specific wording, consider updating/retaining the concentration slot (MIXS:0000102) as deprecated/renamed rather than switching to tot_nitro_content.

Suggested change
- tot_nitro_content
- tot_nitro

Copilot uses AI. Check for mistakes.
Comment on lines +20060 to +20061
- tot_nitro_content
- tot_nitro_cont_meth
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

tot_nitro (removed) was a concentration term for water samples, while tot_nitro_content is defined as a sample “content” term. Replacing it in WastewaterSludge likely changes semantics/expected units; consider keeping a concentration-oriented slot (or adding a new generalized one) instead of mapping to the “content” slot.

Suggested change
- tot_nitro_content
- tot_nitro_cont_meth
- tot_nitro
- tot_nitro_meth

Copilot uses AI. Check for mistakes.
@turbomam
Copy link
Member

We've requested a GitHub Copilot review on this PR as part of a pass across all open MixS PRs. Copilot catches things like unused imports, resource leaks, and naming inconsistencies — it's a lightweight first pass, not a substitute for human review. No action needed from you unless Copilot flags something you agree with.

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

Labels

5-TechnicalUpdates Updates to the LinkML representation or technical components of MIxS

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

term total nitrogen concentration (tot_nitro) is water specific, should be general

5 participants