Conversation
There was a problem hiding this comment.
I think this is very valuable. A few general comments/questions, aside from the ones related to specific content.
- The text is based on our course tutorial. That means we also take time to explain Rust concepts (e.g. traits, generics, etc.). Does that make sense for a tutorial for the main Pumpkin solver? Personally, I lean toward no, but it might make the solver more approachable.
- I assume mention of the course is not in the tutorial? E.g. the repository we link is the course repo instead of the main Pumpkin repo.
- How do we expect the user to interact with pumpkin? If they are writing their own propagators, I would expect them to pull in pumpkin-solver as a dependency into their own crate. Then they can run models, implement search procedures and propagators, etc. That is a very different story than if they clone the Pumpkin repository. This also impacts if we mention clippy, git hooks, etc.
Finally, I think we should aim to avoid norun on code blocks as much as possible. That way, we can be confident that when we run mdbook test, the code still matches a specific Pumpkin version. Since we still frequently update various APIs, the tutorial will become stale very quickly otherwise.
| ## Conflict Check | ||
|
|
||
| If both variables are already assigned to different values, the constraint `a = b` is violated. In general, it is common to check for conflicts before performing propagation, because an early check can often simplify the propagation algorithm that follows. | ||
|
|
||
| ```rust,no_run,noplayground | ||
| if a_lb == a_ub && b_lb == b_ub && a_lb != b_lb { | ||
| return Err(Conflict::Propagator(PropagatorConflict { | ||
| conjunction: conjunction!([self.a == a_lb] & [self.b == b_lb]), | ||
| inference_code: self.inference_code.clone(), | ||
| })); | ||
| } | ||
| ``` |
There was a problem hiding this comment.
This conflict check is too specific. We want the conflict to trigger if the intersection of the domains is empty. If we only consider bounds for that, then we should have the following:
a_lb > b_ub || a_ub < b_lb
| ### TODO | ||
| - use the `fixed_value` function and modify this page accordingly. | ||
| - could reference the conjunction macro documentation | ||
| - this is a bit ugly: `Err(Conflict::Propagator(PropagatorConflict {...}))`. Maybe we could something to make this nicer. No newline at end of file |
There was a problem hiding this comment.
Agreed. We could make a function like so:
return propagor_conflict(conjunction, inference_code);
| ``` | ||
|
|
||
| ### TODO | ||
| - use the `fixed_value` function and modify this page accordingly. |
There was a problem hiding this comment.
I don't think we need to use fixed_value
I value being approachable as a key priority to make it easier for people to get into Pumpkin, even if they do not know Rust. That said, now that you mention it, we could visually separate Rust info from other content, either by placing it in a separate section (say, in the getting started section) and/or by using an emoji at the start of an explanation to indicate that this is for people not familiar with Rust. Question @maartenflippo: Which part of the tutorial do you feel explains too much Rust that it gets in the way? Just trying to see how to approach this issue.
That needs to be fixed indeed, relates to the point below.
Good point. If they just write propagators, then indeed it does not make sense to clone the repository, but only pull in the propagator part. What is the best way forward here?
This is a good point, I will look into this. p.s. I updated the todos. |
|
@maartenflippo Could you update the tutorial code with:
I could look into the other points for the tutorial. |
I tried out putting it in a |
I have been reworking our tutorial for creating propagators (relates to issue #300). The tutorial now looks more Rust-like!
Please have a look, and let me know what you think.
How to view the tutorial
There are two options:
Option 1: static
To just view the draft tutorial, you can open it in your browser
tutorials/first-propagator/book/01-introduction.html.Option 2: dynamic
Alternatively, if you also want to change the files, you can:
cargo install mdbookthen navigate to the folder
tutorials/first-propagatorand typemdbook serve --openThat should open the tutorial in the browser, and as you modify the files in the
srcfolder, it should immediately apply changes.TODOs
There is still quite a bit to go...here is a list of TODOs:
inference_codein the constructor.createinPropagatorConstructorcould be renamed intocreate_propagator, to make it clearer that it creates propagators, and it is not a method for creating constructors. It might be confusing for people, sincecreatesounds like it constructs the PropagatorConstructor, but instead it actually creates the propagator and not the constructor.fixed_value.Conflict::Propagator(PropagatorConflict {...}). We could come up with something to make this look nicer, and change in the tutorial.a_lb > b_ub || a_ub < b_lb)