Repository navigation
Add resource_family for grouping resource types in the segmentation - #108
espenbodal wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
🟡 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_familyand 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.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
JulStraus
left a comment
There was a problem hiding this comment.
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
endyou 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.
There was a problem hiding this comment.
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.
Co-authored-by: Julian Straus <104911227+JulStraus@users.noreply.github.com>
Co-authored-by: Julian Straus <104911227+JulStraus@users.noreply.github.com>

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