Skip to content

feat: Multiple inputs to vector-type parameters - #2189

Merged
trisyoungs merged 30 commits into
develop2from
dissolve2/vectorinput
Jul 14, 2025
Merged

feat: Multiple inputs to vector-type parameters#2189
trisyoungs merged 30 commits into
develop2from
dissolve2/vectorinput

Conversation

@trisyoungs

@trisyoungs trisyoungs commented Jun 27, 2025

Copy link
Copy Markdown
Member

This PR implements handling of std::vectors in Parameters, allowing them to be set via either another vector or to be constructed piecewise from multiple single data item inputs. The nice side effect is that there is now a single Parameter<T> class with no derived specialisations (save for Function1DWrapper which could also be accounted for but would pollute the class a little bit, plus it's slated for addressing at a later stage).

I'll be honest - I went around the houses on this one! No amount of template mangling, overloading, or C++20 sugar would ultimately allow me to follow my initial route which was to move to a Parameter<DataClass,StorageClass> where the storage type was explicitly known and could thus be accounted for with constexpr conditionals, but it turns out that this is (probably) impossible as one or the other template classes is "lost" when you work from functions in ParameterBase (which we necessarily do). However, the C++20 requires keyword came to the rescue, and the end result is something not too far from what I had originally tried to create.

The base upcast() function is gone and the ParameterBase<T>::set() and get() functions are now very type explicit, e.g. passing an int or a double when setting a Number will not work. However, we only do this in the unit tests so no real biggie.

Comment thread src/nodes/parameter.h Outdated
Comment thread src/nodes/parameter.h Outdated
Comment thread src/nodes/parameter.h
requires(std::is_pointer_v<DataClass>)
: ParameterBase(parent, name, description, std::type_index(typeid(DataClass))), data_(localPointer_), default_(nullptr),
dataGetter_([&]() { return targetData.has_value() ? &targetData.value() : nullptr; }),
dataSetter_([](const DataClass &value) { return false; })

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this just a temp value until we fix it at a later date or am I missing a reason why we can't have a setter here? Somethin along the lines of:

if (value == nullptr && targetData.has_value())
{
    targetData = {};
    return true;
}
if (value != nullptr && (!targetData.has_value() || *targetData != value))
{
    targetData = value;
    return true;
}
return false;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

One of the reasons against all this requires usage is the fact that it can obfuscate the intent of things a bit, or at least require you to balance the context with another four overloads at the same time! Here the source data object is a std::optional and basically constitutes a read-only Parameter, hence the setter returns a hard fail.

Comment thread src/nodes/parameter.h

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite Current: 3d4ca4a Previous: 3f79d38 Ratio
BM_Box_MinimumImage<CubicBox> 13.572743355106727 ns/iter 6.235206516096226 ns/iter 2.18

This comment was automatically generated by workflow using github-action-benchmark.

CC: @disorderedmaterials/dissolve-devs

Comment thread src/nodes/parameter.h Outdated
@trisyoungs
trisyoungs marked this pull request as ready for review July 4, 2025 16:40
@trisyoungs trisyoungs changed the title Dissolve2/vectorinput feat: Multiple inputs to vector-type parameters Jul 9, 2025
@trisyoungs
trisyoungs merged commit 8701baf into develop2 Jul 14, 2025
10 of 11 checks passed
@trisyoungs
trisyoungs deleted the dissolve2/vectorinput branch July 14, 2025 08:19
rprospero pushed a commit that referenced this pull request Sep 5, 2025
Co-authored-by: Tristan Youngs <trisyoungs@googlemail.com>
rprospero pushed a commit that referenced this pull request Feb 4, 2026
Co-authored-by: Tristan Youngs <trisyoungs@googlemail.com>
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.

3 participants