Add boolean volumes by gonzaponte · Pull Request #22 · jacg/nain4

gonzaponte · 2023-07-13T07:55:02Z

A simple proposal to close #5.

Pro: extremely simple
Con: creates unnecessary logical volumes

IMHO, the pro outweighs the con.

jacg · 2023-07-13T08:30:11Z

Be a bit more creative in your use of issue closure keywords.

How about changing your first line to

A simple proposal to close #5.

?

Similarly, chuck in a keyword here.

As much as I love all the work you are doing here, and the rate at which you are doing it, reviewing it all is preventing me from making much progress on writing the documentation that this project desperately needs. So, could you please add one or two relevant pages to the docs, that describe this solution, directly in this PR?

Recall that we have four categories of documentation, and something like this would need something in at least How-To and Reference, and maybe even in Tutorial. Writing the reference is likely to be a bit tedious, and it won't be stable until we've finalized the details, so that can be left until later.

Ideally, we want to see the plain G4 version next to the n4 one, to help us evaluate whether it is worth the effort.

Similarly for #14, which I haven't had the time to look at, yet.

In general, we shouldn't add any new utilities without documentation ... and tests, but I know that I don't need to tell you about the tests.

gonzaponte · 2023-07-13T08:38:06Z

Be a bit more creative in your use of issue closure keywords.

I'm aware of this. I tend to add them later on when the PR is more mature, but I'm ok with doing it from the beginning.

could you please add one or two relevant pages to the docs?

Sure thing.

Similarly for #14, which I haven't had the time to look at, yet.

Don't waste your time on that. It's not the most urgent feature and it's not working.

jacg · 2023-07-13T08:36:00Z

+  auto vsum  = 2 * vbox - vint;
+  auto vsub  =     vbox - vint;
+
+  auto box1 = nain4::volume<G4Box>("test_box1",      water, l/2, l/2, l/2);


Consider using the n4 synonym for the nain4 namespace, for brevity.

jacg · 2023-07-13T08:44:13Z

+  auto items = { make_tuple(union1, vsum, "add")
+               , make_tuple(union2, vsum, "join")
+               , make_tuple(subtr1, vsub, "sub")
+               , make_tuple(subtr2, vsub, "subtract")
+               , make_tuple(inter1, vint, "int")
+               , make_tuple(inter2, vint, "intersect")
+               };
+
+  auto density = water->GetDensity();
+  for (auto& item : items){


Have you considered Catch2 generators here?

I did, but the keywords ADD, etc. are class names and I couldn't make it work.

jacg · 2023-07-13T08:52:50Z

+  auto union1 = nain4::boolean<ADD      >("test_add"      , box1, box2, norot, dr);
+  auto union2 = nain4::boolean<JOIN     >("test_join"     , box1, box2, norot, dr);
+  auto subtr1 = nain4::boolean<SUB      >("test_sub"      , box1, box2, norot, dr);
+  auto subtr2 = nain4::boolean<SUBTRACT >("test_subtract" , box1, box2, norot, dr);
+  auto inter1 = nain4::boolean<INT      >("test_int"      , box1, box2, norot, dr);
+  auto inter2 = nain4::boolean<INTERSECT>("test_intersect", box1, box2, norot, dr);


I don't like the fact that null values such as norot have to be passed in explicitly.

Maybe we can use a builder pattern (like in n4::place), to mitigate this. OTOH, there are far fewer parameters here, so it might not be worth the effort.

It's not immediately obvious whether the builder pattern would play nicely with the template arguments.

In the constructors for boolean solids there are only two options: rotation + translation or transform. nain4::boolean just forwards the parameters. Do you have any suggestions?

A builder which, when told to build, inspects the internal state it has accumulated, and forwards it to the G4 constructor that is appropriate. Or the most general one, filling in the blanks with null operations.

Looking at this it looks like you've overlooked the rotation matrix?

Also, looking at the 2nd and 3rd constructor in there, it looks like there are mutually exclusive alternatives: transform-3d vs 3-vec and rot-matrix. So the builder would have to prevent you from mixing them, at compile time.

More generally, at some stage I have to take a step back and see if there's a bearable way of writing typestate patterns in C++. I really want to promote the multitude of G4 pitfalls to compile-time errors.

A builder which, when told to build, inspects the internal state it has accumulated, and forwards it to the G4 constructor that is appropriate. Or the most general one, filling in the blanks with null operations.

I understand. I will have to try it and see.

Looking at this it looks like you've overlooked the rotation matrix?

I don't follow. The norot variable in the test is there precisely because I need to pass it as an argument.

The `norot' variable in the test is there precisely because I need to pass it as an argument.

Sorry, brain fart.

jacg · 2023-07-13T08:56:12Z

+  auto union1 = nain4::boolean<ADD      >("test_add"      , box1, box2, norot, dr);
+  auto union2 = nain4::boolean<JOIN     >("test_join"     , box1, box2, norot, dr);
+  auto subtr1 = nain4::boolean<SUB      >("test_sub"      , box1, box2, norot, dr);
+  auto subtr2 = nain4::boolean<SUBTRACT >("test_subtract" , box1, box2, norot, dr);
+  auto inter1 = nain4::boolean<INT      >("test_int"      , box1, box2, norot, dr);
+  auto inter2 = nain4::boolean<INTERSECT>("test_intersect", box1, box2, norot, dr);


It would be good if the name were optional, with a default being derived from the names of the two constituent volumes and the operation being applied to them.

jacg · 2023-07-13T09:03:01Z

+  auto union1 = nain4::boolean<ADD      >("test_add"      , box1, box2, norot, dr);
+  auto union2 = nain4::boolean<JOIN     >("test_join"     , box1, box2, norot, dr);
+  auto subtr1 = nain4::boolean<SUB      >("test_sub"      , box1, box2, norot, dr);
+  auto subtr2 = nain4::boolean<SUBTRACT >("test_subtract" , box1, box2, norot, dr);
+  auto inter1 = nain4::boolean<INT      >("test_int"      , box1, box2, norot, dr);
+  auto inter2 = nain4::boolean<INTERSECT>("test_intersect", box1, box2, norot, dr);


IIUC, your solution always combines two volumes to produce a third. What happens if I want to use that in a further boolean operation?

In the very vague thoughts I had about this when opening #5, I was thinking of an interface that would allow chaining, or even arbitrary trees of operations.

As I don't recall having used these boolean operations myself in real code, I have no clue whether real Geant4 use cases have a need for this, even though it would be the right the to do from an idealistic perspective.

What happens if I want to use that in a further boolean operation?

I haven't tried, but I wouldn't expect a problem with that. Maybe I'm wrong.

I'm not expecting any technical problems, just verbosity. So the question is whether we can come up with a better interface for chaining boolean operations.

I think we can. But I have the feeling that the final interface can be decoupled from this implementation. We can offer something similar to place that uses this functionality behind the scenes.

Well, yes, this would be an implementation details of the interface. So the client would end up not seeing this at all.

We also need to have a good look at G4MultiUnion.

We have to be wary of creating beautiful, convenient interfaces, which generate inefficient code and promote bad practices.

We also need to have a good look at G4MultiUnion.

I'm aware of this, but I postponed its implementation to a later stage since it needs special care.

jacg · 2023-07-13T09:07:21Z

I tend to add them later on when the PR is more mature

Given that the point of the PR is to close that issue, I'd say it should go in from the beginning.

jacg · 2023-07-13T10:33:42Z

Con: creates unnecessary logical volumes

I guess there's no reason for this, other than you reusing n4::volume?

Given that all this boolean solid stuff uses G4VSolids as inputs and outputs, there really shouldn't be any logical volumes at all.

I'm thinking about the more general interface discussed in one of the review conversations, and I don't see any reason why there should be any logical volumes.

jacg · 2023-07-13T10:48:18Z

What would be the problems with an interface like this:

G4VSolid* first = ...;
// etc.
G4VSolid* funky_solid = n4::boolvol(first)
    .add(second).at(position).rot(rotaion)
    .sub(third).trans(transform_3d)
    .name("Optional override of default name constructed from components and ops")
    .now();

?

gonzaponte · 2023-07-13T11:40:29Z

I guess there's no reason for this, other than you reusing n4::volume?

Correct.

What would be the problems with an interface like this

I don't see a problem. But I do appreciate skipping the somewhat unnecessary step of creating a solid that adds more noise than signal.

jacg · 2023-07-13T11:47:41Z

And it would subsume the use cases from your tests, e.g.

auto union1 = n4::boolean<ADD>("test_add", box1, box2, norot, dr);

would become

auto union1 = n4::boolean(box1).add(box2).at(dr).now();
auto onion1 = n4::boolean(box1).add(box2).at(dr).name("test_add").now(); // If you *really* want the name

jacg · 2023-07-13T11:51:29Z

But I do appreciate skipping the somewhat unnecessary step of creating a solid that adds more noise than signal.

Indeed. So .now() can be bifurcated into two methods, one which returns a solid, one which gives you the logical volume. Ideas for pithy but sufficiently clear names?

jacg · 2023-07-13T11:55:54Z

Ideas for pithy but sufficiently clear names?

{solid,logical}
get_{solid,logical}
make_{solid,logical}
{now,logical}

gonzaponte · 2023-07-13T11:57:25Z

I do like the syntax though. I think it is very clear and self-explanatory. The main inconvenience I find at the moment is that the input solids need to be created explicitly. So we either add an interface in n4 (which doesn't add anything over G4?) or we let the user create them in G4 (which looks dirty to me).

So .now() can be bifurcated into two methods

Is there a case for returning solids? Or is it just for flexibility?

jacg · 2023-07-13T12:10:59Z

Is there a case for returning solids?
Depends on whether we find a nice interface for constricting more complex boolean hierarchies. With .solid() it just works:

G4VSolid* this = n4::boolean(...). ... .solid();
G4VSolid* that = n4::boolean(...). ... .solid();
return n4::boolean(this).add(that).logical();

Or is it just for flexibility?

It would be trivial to add, so even if it were only that, it's probably worth while.

The main inconvenience I find at the moment is that the input solids need to be created explicitly. So we either add an interface in n4 (which doesn't add anything over G4?) or we let the user create them in G4 (which looks dirty to me).

How about

n4::boolean<G4Sphere>("mihuevo-L", ...)
       .add<G4Shpere>("mihuevo-R", ...).at(dr)
       .name("mishuevos")
       .logical();

?

I guess we can overload the templates with non-templated versions which still take pre-constructed solids.

gonzaponte · 2023-07-13T12:18:25Z

Is there a case for returning solids?

Depends on whether we find a nice interface for constricting more complex boolean hierarchies. With .solid() it just works:

Yes, I agree. It can help the assembly of complex boolean compositions.

How about ...

I like it. In practice, the case of pre-constructed solids will probably prevail, but that's as much as can be done, I think.

jacg · 2023-07-13T12:19:03Z

Depends on whether we find a nice interface for constricting more complex boolean hierarchies. With .solid() it just works

Actually, .add() and friends could accept an n4::boolean, so we wouldn't need .solid() for that use case. But still, it's trivial, and adds a bit of flexibility at very little cost.

gonzaponte · 2023-07-13T12:21:00Z

So to summarize, we are going directly to this interface, right?

jacg · 2023-07-13T12:22:33Z

In practice, the case of pre-constructed solids will probably prevail

Hmm ... maybe I'm not fully understanding what you are saying.

So to summarize, we are going directly to this interface, right?

Yes, I think so.

jacg · 2023-07-13T12:30:01Z

Ooops, the proposed interface doesn't mention materials, which are needed if we want it to return logical volumes.

Maybe a .material() method, which turns the product from solid to logical? But then you would need to ensure that it's given at each step ... preferably at compile time ... which would be a tricky typestate to code in C++.

jacg · 2023-07-13T12:35:33Z

Ah, it's OK: the whole thing must have the same material, right?. So

.solid()
.logical(material)

would be enough.

We could even do

.now() for solid
.now(material) for logical

but that's probably too cute and cryptic.

gonzaponte · 2023-07-13T12:39:00Z

Hmm ... maybe I'm not fully understanding what you are saying.

The templated methods you proposed can become difficult to read relatively easily, I think. So the natural choice would be to create the input solids separately and then feed them into n4::bolean. Then we go back to

So we either add an interface in n4 (which doesn't add anything over G4?) or we let the user create them in G4 (which looks dirty to me).

Regarding the whole material discussion, I like your solution.

jacg · 2023-07-13T13:19:16Z

The templated methods you proposed can become difficult to read relatively easily, I think.

You mean because of the bazillion arguments that they take, they would drown out the structure with low-level noise? Yes, quite possibly. So do you think we should not provide them?

So we either add an interface in n4 (which doesn't add anything over G4?)

I do think that

n4::boolean(thys).add(that).at(position).logical(water);

is a huge improvement over ... well, I can't be bothered to figure out the equivalent bare G4.

[Aside: looking at the above, it might be nice if the end were spelt .material(water), but perhaps that doesn't make it sufficiently obvious that a logical volume is being made at this point.]

or we let the user create them in G4 (which looks dirty to me).

I don't think there is much that can be done to improve the situation: these solids do need a bunch of different construction arguments, and they have to be passed in somewhere. If all you're doing is creating a simple solid, then I don't think n4 can improve over G4 in any significant way.

So I think it's perfectly OK for the earlier example to be preceded with

auto thys = new G4Sphere(lots, of, noisy, arguments, that, must, be, given, somewhere);

gonzaponte · 2023-07-13T13:31:30Z

So we either add an interface in n4 (which doesn't add anything over G4?)

I do think that
n4::boolean(thys).add(that).at(position).logical(water);
is a huge improvement

I agree. I meant an n4 interface to create thys and that, which are solids. As you point out, I don't think n4 can provide anything better than G4, which annoys me a bit. And mixing G4 with n4 in the client code feels dirty.

it might be nice if the end were spelt .material(water), but perhaps that doesn't make it sufficiently obvious that a logical volume is being made at this point

I kinda like logical(material). It almost reads "build a logical volume with this material".

jacg · 2023-07-13T13:41:55Z

I don't think n4 can provide anything better than G4, which annoys me a bit.

Te-he. Me too. Just a little bit. But this is an example of where G4 doesn't do anything terribly wrong. Probably because it's difficult to imagine how to mess up something so simple.

[Hmm, now you've got me thinking about whether these solids' multiple parameters can be organized more conveniently.]

And mixing G4 with n4 in the client code feels dirty.

I know what you mean, but we have to be pragmatic and not excessively purist. n4 tries to help you write G4 code, where it can: it's not a complete replacement for G4. I think it's an important message to send to potential users, that n4 does not force you into anything: the whole of G4 (familiarity, warts, bugs and all) remains available to you, but we can help to make some things easier, clearer and more robust.

gonzaponte · 2023-07-13T14:31:10Z

What's the reason for child to be optional in n4::place?

jacg · 2023-07-13T14:59:15Z

What's the reason for child to be optional in n4::place?

Hmm. Two things, I guess

There is no way of preventing the client from passing in a nullptr
You might want to specify where things will be placed, earlier, and later on say what should be placed there.
```
auto put_somewhere_special = n4::place().in(mother).at(position).rot(rotation);
auto my_precious_thing = ... // complex logic
put_somewhere_special.child(my_precious_thing).now();
```
Pay careful attention to where .now() is written (and, perhaps more importantly, where it is not).
Of course, for this to work, we'd need a .child() method (with some good name), and there isn't one, yet.

More generally, nullptr doesn't distinguish between

This hasn't been specified
This has been specified to be NULL

and option does.

So all our, as yet unimplemented, typestates should give compile-time errors if you try to build before all of the mandatory values have been set.

In other words, it's probably there, setting us up for the conceptually correct thing to do, but a number of things have not been implemented in this case, yet.

Does this make any sense?

gonzaponte · 2023-07-13T15:23:12Z

Yes. I was slightly confused because I didn't see any reason for a childless case, mostly. So if IIUC, at the moment optional's job is to protect against nullptrs?

jacg · 2023-07-13T15:28:19Z

No, I don't think it protects against nullptrs at the moment. The constructor accepts them, and .now() doesn't perform any checks.

gonzaponte · 2023-07-13T15:30:13Z

No, but it gives you an error, instead of a segfault, which is a small improvement.

jacg · 2023-07-27T14:13:53Z

I think that with all the n4::solids and their interfaces, we've answered most of the interface design doubts we were having above.

Let's get this done now, so that we can see if the solid API can accommodate it sooner rather than later.

gonzaponte added 3 commits July 13, 2023 10:02

Add boolean logical volumes

3bf735f

Add tests for boolean

b2cc2b4

std::make_tuple -> make_tuple

68de68c

jacg reviewed Jul 13, 2023

View reviewed changes

jacg mentioned this pull request Jul 27, 2023

Better interfaces for constructing G4 Solids #25

Closed

gonzaponte closed this in 4610001 Jul 28, 2023

gonzaponte deleted the boolean-volumes branch August 9, 2023 15:44

Conversation

gonzaponte commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacg commented Jul 13, 2023

Uh oh!

gonzaponte commented Jul 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacg commented Jul 13, 2023

Uh oh!

jacg commented Jul 13, 2023

Uh oh!

jacg commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gonzaponte commented Jul 13, 2023

Uh oh!

jacg commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacg commented Jul 13, 2023

Uh oh!

jacg commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gonzaponte commented Jul 13, 2023

Uh oh!

jacg commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gonzaponte commented Jul 13, 2023

Uh oh!

jacg commented Jul 13, 2023

Uh oh!

gonzaponte commented Jul 13, 2023

Uh oh!

jacg commented Jul 13, 2023

Uh oh!

jacg commented Jul 13, 2023

Uh oh!

jacg commented Jul 13, 2023

Uh oh!

gonzaponte commented Jul 13, 2023

Uh oh!

jacg commented Jul 13, 2023

Uh oh!

gonzaponte commented Jul 13, 2023

Uh oh!

jacg commented Jul 13, 2023

Uh oh!

gonzaponte commented Jul 13, 2023 •

edited

Loading

jacg commented Jul 13, 2023 •

edited

Loading

jacg commented Jul 13, 2023 •

edited

Loading

jacg commented Jul 13, 2023 •

edited

Loading

jacg commented Jul 13, 2023 •

edited

Loading