Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add tracing for head arithmetic #374

Merged
merged 4 commits into from
Oct 18, 2023
Merged

Add tracing for head arithmetic #374

merged 4 commits into from
Oct 18, 2023

Conversation

aannleax
Copy link
Member

Allows tracing for rules that compute new values in the head. For example:

project_reject("Wind Turbine B") :- distance("Wind Turbine B", 30.965), 30.965 < 200 .
 └─ distance("Wind Turbine B", 30.965) :- project_location_m("Wind Turbine B", 1525632.5746, 4578188.0003), positions_m(1525619.4715, 4578216.0563), 30.965 = sqrt((1525632.5746 - 1525619.4715) * (1525632.5746 - 1525619.4715) + (4578188.0003 - 4578216.0563) * (4578188.0003 - 4578216.0563)) .
    ├─ project_location_m("Wind Turbine B", 1525632.5746, 4578188.0003) :- project_location("Wind Turbine B", 13.7049, 50.8982), 1525632.5746 = 13.7049 * 111320, 4578188.0003 = 50.8982 * 89948 .
    │  └─ project_location("Wind Turbine B", 13.7049, 50.8982)
    └─ positions_m(1525619.4715, 4578216.0563) :- locations(13.7048, 50.8985, "856", "1913826124", "", "", "", "", "", "", "", "", ""natural"=>"tree""), 1525619.4715 = 13.7048 * 111320, 4578216.0563 = 50.8985 * 89948 .
       └─ locations(13.7048, 50.8985, "856", "1913826124", "", "", "", "", "", "", "", "", ""natural"=>"tree"")

@aannleax aannleax added this to the Release 0.4.0 milestone Oct 17, 2023
mmarx
mmarx requested changes Oct 17, 2023
View reviewed changes
Copy link
Member

@mmarx mmarx left a comment

Choose a reason for hiding this comment

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

I'm not confident that evaluating everything as f64 is the correct thing to do here.

nemo/src/model/rule_model/term.rs Outdated
@@ -367,6 +375,58 @@ impl Term {
Term::Function(sub) => sub.subterms.iter().any(Self::aggregate_subterm_recursive),
}
}

/// Evaluates a constant (numeric) term
/// We do this by casting everything to `f64` as it seems like the most general number type.
Copy link
Member

Choose a reason for hiding this comment

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

It's really not, for example, 9007199254740992.0 + 1.0 == 9007199254740992.0 holds for f64, but surely not for i64.

Copy link
Member Author

Choose a reason for hiding this comment

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

I could add something like https://crates.io/crates/bigdecimal for "compile time" arithmetic.

Maybe not for this PR though as this as this is a feature that is needs to demonstrated next week and its good enough

Copy link
Member Author

Choose a reason for hiding this comment

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

It turns out I had the type information there all along and its not too complicated to use the evaluation code from the physical layer for the evaluation here

nemo/src/model/rule_model/term.rs Outdated
BinaryOperation::Addition(_, _) => Some(value_left + value_right),
BinaryOperation::Subtraction(_, _) => Some(value_left - value_right),
BinaryOperation::Multiplication(_, _) => Some(value_left * value_right),
BinaryOperation::Division(_, _) => Some(value_left / value_right),
Copy link
Member

Choose a reason for hiding this comment

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

if value_left = value_right = 0, this will produce a Double holding a NaN, which shouldn't happen, and since we currently only have checks for NaN in the Double constructors, this will just lead to weird behaviour later on.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I could do the checked_ versions for all the operations here

@aannleax aannleax linked an issue Oct 17, 2023 that may be closed by this pull request
Support tracing for rules with constructors #373
Closed
@aannleax aannleax requested a review from mmarx October 17, 2023 11:08
@aannleax aannleax linked an issue Oct 18, 2023 that may be closed by this pull request
Tracing should not use floats for evaluating terms #377
Closed
@aannleax aannleax merged commit 6d62a25 into main Oct 18, 2023
@mmarx mmarx deleted the feature/373-tracing-arithmetic branch October 18, 2023 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmarx mmarx mmarx approved these changes

Assignees
No one assigned
Labels
None yet
Projects
nemo
Archived in project
Milestone
Release 0.4.0
Development

Successfully merging this pull request may close these issues.

Tracing should not use floats for evaluating terms Support tracing for rules with constructors
2 participants
@aannleax @mmarx