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.

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

added unknown docs #6839

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

added unknown docs #6839

wants to merge 7 commits into from

Conversation

sanderr
Copy link
Contributor

@sanderr sanderr commented Dec 7, 2023

Description

Added documentation about unknowns.

closes #6056

Self Check:

Strike through any lines that are not applicable (~~line~~) then check the box

  • Attached issue to pull request
  • Changelog entry
  • Type annotations are present
  • Code is clear and sufficiently documented
  • No (preventable) type errors (check using make mypy or make mypy-diff)
  • Sufficient test cases (reproduces the bug/tests the requested feature)
  • Correct, in line with design
  • End user documentation is included or an issue is created for end-user documentation (add ref to issue here: )
  • If this PR fixes a race condition in the test suite, also push the fix to the relevant stable branche(s) (see test-fixes for more info)

@sanderr sanderr requested review from wouterdb and edvgui December 7, 2023 14:59
docs/language.rst Outdated Show resolved Hide resolved
Unknowns are a subtle concept. Luckily, for the majority of model development you don't really need to take them into
account. However, for some advanced scenarios it may be important to know how and where they may occur.

For the most part, unknowns are simply propagated along the data flow: statements like assignment statements, constructors
Copy link
Contributor

Choose a reason for hiding this comment

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

I would first give the simple explanation

Suggested change
For the most part, unknowns are simply propagated along the data flow: statements like assignment statements, constructors
For the most part, unknowns are simply propagated along the data flow: any value derived from an unkown in any way becomes an unknown as well. More specifically: statements like assignment statements, constructors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, but I'm going to move it one sentence later, because it really applies to the "any expression that cannot produce ..." part. e.g. [1, unknown, 3] does not become unknown.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made an attempt for a middle ground, to give some more context upfront, but in a way that applies for both statements that follow.

docs/language.rst Outdated Show resolved Hide resolved
end

for x in [1, 2, my_unknown]:
# this block is executed twice: x=1 and x=2
Copy link
Contributor

Choose a reason for hiding this comment

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

my opinion on this is known. I would like to still have this power, but I leave it to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By now the behavior is no longer under discussion, it's simply about documenting it. I'll double check this now.

end

g = my_unknown ? true : false # condition is unknown -> neither branch is executed, result is unknown
h = [1 for x in [1, 2, my_unknown]] # the expression `1` is executed once with x=1 and once with x=2. Unknown is propagated as is -> h = [1, 1, unknown]
Copy link
Contributor

Choose a reason for hiding this comment

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

I find these two to be hard to reconcile with the for loop. For me, a list comprehension is

result = [a for item in c if d]

with a and d expressions referring to item and c an expression

collector = ...
for item in c:
  if d:
    collector.collection += a
result = collector.collection

but again, I raised this before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doing a constructor call here is quite common I think. The 1 is just to keep the model simple.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll update this to construct a simple entity.

# an unknown can even represent more than one unknown value
b = my_unknown == 0 ? [1, 2] : [3, 4] # b = unknown -> when it becomes known it will be either [1, 2] or [3, 4]

c = std::len(l) # c = unknown (l contains unknowns, so its length is also unknown)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would also remark the risk of having unknowns in conditions

implementation things_are_on for A:
   self.b.is_on = self
end

implementation things_are_off for  A:
     self.b.is_off = self
end

implement A using things_are_on if self.on
implement A using things_are_off if self.off


collector = B()
all = [
 A(b=b, on=True)
  A(b=b, on=False)
   A(b=b, on=unknown())
 ]

std::len(b.is_on) # 1  
std::len(b.is_off) # 1
# the fact that the length of b.is_on and b.is_off are unknown is lost
# the intuitive notion that all things are the things that are off + the things tat are on no longer holds 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realize it's been a very long time. But do you remember what you were getting at here? Two notes:

  1. the model had some gaps. I reworked it in the following format that does compile
entity A:
    bool on
end

implementation things_are_on for A:
    self.b.is_on = self
end

implementation things_are_off for A:
    self.b.is_off = self
end

implement A using things_are_on when self.on
implement A using things_are_off when not self.on

entity B:
end
implement B using std::none
A.b [1] -- B
B.is_on [0:] -- A
B.is_off [0:] -- A


b = B()
all = [
    A(b=b, on=true),
    A(b=b, on=false),
    A(b=b, on=std::get_env("THIS_ENV_VAR_DOES_NOT_EXIST")),
]


on_len = std::len(b.is_on)
off_len = std::len(b.is_off)
on_len_txt = std::is_unknown(on_len) ? "unknown" : on_len
off_len_txt = std::is_unknown(off_len) ? "unknown" : off_len
std::print(f"b.is_on = {b.is_on}")
std::print(f"b.is_off = {b.is_off}")
std::print(f"std::len(b.is_on) = {on_len_txt}")  # 2
std::print(f"std::len(b.is_off) = {off_len_txt}")  # 2

# THE BELOW APPEARS TO BE INCORRECT
std::len(b.is_on) # 1
std::len(b.is_off) # 1
# the fact that the length of b.is_on and b.is_off are unknown is lost
# the intuitive notion that all things are the things that are off + the things tat are on no longer holds
  1. This behavior suprises me a lot. It looks like when treats unknowns differently than if does. I'm inclined to conclude that that's a bug for when, rather than a cautionary note for the model developer.

@wouterdb
Copy link
Contributor

wouterdb commented Dec 7, 2023

Nice!

Copy link
Contributor

@edvgui edvgui left a comment

Choose a reason for hiding this comment

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

Very nice!

docs/language.rst Show resolved Hide resolved
@sanderr
Copy link
Contributor Author

sanderr commented Jan 16, 2025

It's been a while, but I finally found the time to address review comments.

@sanderr sanderr requested review from edvgui and wouterdb January 16, 2025 14:18
Copy link
Contributor

@edvgui edvgui left a comment

Choose a reason for hiding this comment

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

Lgtm. I was wondering if it wouldn't add value to the example models to augment them with std::assert(std::is_unknown(...), "") because at the moment, the model will be valid (it will compile) whether the unknowns actually behave as advertised or not, and the only thing illustrating that something is unknown is its name. I could hear a different take on this though, this is not a test case after all.

@sanderr
Copy link
Contributor Author

sanderr commented Jan 16, 2025

I considered it, but I found it too verbose for the docs. That said, I should probably add a test case that loads the snippets.

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.

document Unknown semantics
3 participants