Skip to content

Add resource_family for grouping resource types in the segmentation - #108

Open
espenbodal wants to merge 5 commits into
mainfrom
enhanc/resource_family
Open

espenbodal wants to merge 5 commits into
mainfrom
enhanc/resource_family

Conversation

@espenbodal

Copy link
Copy Markdown
Member

Closes #106. Adds resource_family(p::Resource), defaulting to typeof(p), and uses it in res_types and res_types_vec. No behaviour change for existing resources; see the issue for the motivation. Includes tests, NEWS entry and documentation.

@espenbodal
espenbodal requested review from JulStraus and a balanced review from Copilot October 8, 2026 13:01

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The public API page references an unexported function without qualification, which can break documentation generation.

1 open finding
What changed in this PR

Adds resource-family grouping so related resource types can share segmentation and resource-specific model hooks.

Changes:

  • Adds resource_family and updates segmentation.
  • Adds integration tests.
  • Documents the extension API and release note.
File Description
src/​structures/​resource.jl Implements family-based segmentation.
test/​test_resource.jl Tests grouping and model-hook invocation.
docs/​src/​library/​public/​resources.md Adds API documentation.
docs/​src/​how-to/​extend-resource-functionality.md Explains family extension usage.
NEWS.md Records the new API.

🧠 Review effort: Balanced


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Comment thread docs/src/library/public/resources.md Outdated
espenbodal and others added 2 commits October 8, 2026 15:05
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

@JulStraus JulStraus left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See my comments. Note that the current module suggestion from Copilot is bullshit. That is not the structure we want to have. If a function is not exported, it is not in the external library, that is the simple rule.

Additional comment:
We are currently in a situation with additional variables that it always only create one set of variables per resource, even if it theoretically could get them from a higher hierarchy as we use it for Nodes, Links, or ExtensionData. Do we see a case where this is not sufficient?

I.e., for your example,

abstract type AbstractPotentialPower <: Resource end
struct PotentialPower <: AbstractPotentialPower
    # fields as above
end
struct BoundedPotentialPower <: AbstractPotentialPower
    # fields with different bounds
end

you can either create variables for PotentialPower and BoundedPotentialPower (if you do not add the new method) or AbstractPotentialPower. But never variables for PotentialPower and AbstractPotentialPower simultaneously. In this case, we must follow the approach used for, e.g., Node.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Put it under the header Extension functions in the internal library. My plan is to clean up a bit in the internal library and that is something users should extend. You can also remove the text around it as it is explained in the documentation.

Or do you foresee that this function should be used directly in other packages? I honestly do not see the requirement yet.

Comment thread src/structures/resource.jl
Comment thread NEWS.md Outdated
espenbodal and others added 2 commits October 8, 2026 16:28
Co-authored-by: Julian Straus <104911227+JulStraus@users.noreply.github.com>
Co-authored-by: Julian Straus <104911227+JulStraus@users.noreply.github.com>

This branch has not been deployed

No deployments
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.

resource_family for the resource segmentation

3 participants