Add back `OneOf` Value constraints

balacij commented 3 years ago

The fact that removing those constraints does not alter stable feels like a bug. That information should be used, but apparently it's not. So I think the pragmatic thing to do is to remove Enumerated but create two high-priority issues to bring the information thus cut out back. Note that a first hack of bringing the information back could be display-only. #2118 is hard because it tries to deal with code generation, and that design is highly non-trivial. So the high-priority would be to get things back at least for display.

Originally posted by @JacquesCarette in https://github.com/JacquesCarette/Drasil/issues/2646#issuecomment-873109412

balacij commented 2 months ago

As a reminder, this ticket is about building constraints on quantities, specifically 'this quantity must be one of these values' constraints.

@NoahCardoso To re-iterate our intended work:

You have prototyped (are prototyping?) what the 'generated' code should look like for constraints that bind variables to one of many possible values. After this work, we can dissect what is going on and what we really want to be saying (and having generated for us) when we define these constraints.

As of right now, we decided that this should involve sets in the generated code, but we can come back to it later to make sure.

As such,

We have a critical piece missing: sets in *Expr. We will need to bridge this gap in all Exprs and in drasil-gool (@B-rando1 can assist with GOOL :slightly_smiling_face:).
We can finally define the constraint as something that 'means' x \in {... , values, ...}.

Finally,

This work should appear in GlassBr's nomThick, which currently describes this constraint in the description of the nominal thickness quantity and which is not actually used in code generation (see #3818 for more details).

NoahCardoso commented 2 months ago

Here are the prototyped examples in Python and Java and how I think they should look like

JacquesCarette commented 2 months ago

It all looks good - except that enumerated floats are a bad idea. Once in a while, things will go wrong. I would much rather use integers (i.e. multiply everything by 10) for the internal storage of these numbers.

NoahCardoso commented 1 month ago

This is what I have so far. I'm currently just working on making sure it is displayed properly. How should I go about creating sets in GOOL? I have been able to get the Python example working with sets as a constraint but I have not yet added to GOOL. I assume I would need an isIn operation as well as defining Sets

I have a WIP branch open #3846. It's pretty rough currently but I will clean it up once I can get things working.

NoahCardoso commented 1 month ago

Update

I have got sets and the in operation working in Python. Within the next day or so I should be able to get it working in the other languages.

NoahCardoso commented 1 month ago

This is what I have for the C++ example however I am not sure if we should use sets in C++. @balacij mentioned that the set API seems to be discouraged. https://lafstern.org/matt/col1.pdf

JacquesCarette commented 1 month ago

This is looking good. My C++ is too rusty for me to offer advice on which API to use. I'd have to use the same tools you have to investigate (i.e. SE, google, chatGPT, etc)

balacij commented 1 month ago

FWIW, if a language has some missing features, and we can polyfill them with our own generated code, I see no reason why we shouldn't do that. For example (and I'm not saying we should strive for perfection in this PR, but this is a hypothetical follow-up), we can define our own contains operation in a drasil_polyfill shared library.

template <typename E>
bool set_contains(const std::set<E>& set, const E& element) {
    return set.find(element) != set.end();
}

At least for now, I think you're making the right choice in looking to move the set into a variable.

balacij commented 1 month ago

Reading that file I sent now, I think we should prefer a std::set over a vector at least, for readability. The performance considerations of set v vector would be preemptive. So, I'd call set the correct data type to use now. I just wanted to make sure because I found the set API to be quite lacking (no contains seems quite odd to me).

JacquesCarette / Drasil

Add back `OneOf` Value constraints #2655

Update