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

Add back OneOf Value constraints #2655

Closed
balacij opened this issue Jul 5, 2021 · 9 comments · Fixed by #3878
Closed

Add back OneOf Value constraints #2655

balacij opened this issue Jul 5, 2021 · 9 comments · Fixed by #3878
Assignees

Comments

@balacij
Copy link
Collaborator

balacij commented Jul 5, 2021

Related to: #2646 (comment) , #2646 (comment) , and #2118

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 #2646 (comment)

@balacij
Copy link
Collaborator Author

balacij commented Jul 4, 2024

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:

  1. 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,
2. 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 🙂).
3. We can finally define the constraint as something that 'means' x \in {... , values, ...}.

Finally,
4. 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
Copy link
Collaborator

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

@JacquesCarette
Copy link
Owner

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 NoahCardoso mentioned this issue Jul 10, 2024
@NoahCardoso
Copy link
Collaborator

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

image

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
Copy link
Collaborator

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.
image

@NoahCardoso
Copy link
Collaborator

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
image

@JacquesCarette
Copy link
Owner

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
Copy link
Collaborator Author

balacij commented Jul 23, 2024

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
Copy link
Collaborator Author

balacij commented Jul 23, 2024

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).

@balacij balacij changed the title Add back Enumerated Value constraints Add back OneOf Value constraints Jul 23, 2024
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 a pull request may close this issue.

3 participants