Skip to content

fix: FdFirst options and refactor CFDRelationData class#771

Open
Oddin60F wants to merge 2 commits into
Desbordante:mainfrom
Oddin60F:fdfirst_options
Open

fix: FdFirst options and refactor CFDRelationData class#771
Oddin60F wants to merge 2 commits into
Desbordante:mainfrom
Oddin60F:fdfirst_options

Conversation

@Oddin60F

@Oddin60F Oddin60F commented May 27, 2026

Copy link
Copy Markdown
Collaborator

FdFirst options:
Added parameter verification during loading. Previously, an incorrect parameter caused an error when running the algorithm, not when loading the parameters.
CFDRelationData:
The number of rows and columns parameters for loading a part of the table into the algorithm has been removed.

@Oddin60F Oddin60F requested a review from wildsor May 27, 2026 16:38
DESBORDANTE_OPTION_USING;

auto check_num_cols = [this](unsigned int val) {
if (OptionIsSet(kCfdTuplesNumber) && (tuples_number_ != 0 && val == 0)) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Use SetConditionalOpts with empty condition, like in MetricVerifier (kMetric option)

Comment thread src/core/config/option.h
// predicate is equivalent to an always-true predicate, and thus must
// always be last.
Option& SetConditionalOpts(OptCondVector opt_cond) {
assert(opt_cond_.empty());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Shouldn't be removed

@wildsor

wildsor commented May 27, 2026

Copy link
Copy Markdown
Collaborator

The assert says that SetConditionalOpts must only be called once (notice the underscore at the end of the name).

@Oddin60F Oddin60F linked an issue May 29, 2026 that may be closed by this pull request
@Oddin60F Oddin60F changed the title fix: FdFirst options fix: FdFirst options and refactor CFDRelationData class May 29, 2026
@Oddin60F Oddin60F requested a review from wildsor June 4, 2026 18:58
@wildsor

wildsor commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

Could you be more specific on what was done to CFDRelationData? It looks like you did one thing, and then it pulled in a bunch of others? Also, the first commit contains stuff that gets removed in the next one. I think it'd make it easier to review if you split the commits in a way that reflects what changes happened.

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.

FDFirstAlgorithm: fail on incorrect option values early

2 participants