Problem 2305

Hi Simon,

Sorry for my late answer and thanks for this report. 
Yes, I agree, it is a bug.

However, I am trying to remember why I introduced that mapOfMatPropVects and I have a feeling it was addressing some other problem, but I don't know what anymore.
Please give me a few more days to try to recall where it comes from. If it was only for performance reasons, that your fix is certainly fine.

Cheers,
Witek

Hi Witek, 

I am glad you agree.
It would be useful to document the list of releases that have this issue in this ticket together with those without it.

Thanks,  Simon

Hi Simon,

I am bit reluctant to get rid of this map, because in case there was a large number of optical surfaces using the same material property vector, we would create one object for each of them. No big deal, but not very elegant either.

As you have also mentioned, if we used the appropriate key, the problem would be solved. Would then using 'ref' as the key instead of 'Strip(name)' work for you?

So, we would have:

if ( mapOfMatPropVects.find(ref) == mapOfMatPropVects.end())
{
...
...
 mapOfMatPropVects[ref] = propvect;
...
}
else
{
 propvect = mapOfMatPropVects[ref];
}

Would that be OK for you?

As far as the affected versions are concerned, I have introduced that map in the tag 10-05-ref-05, which means that all the releases starting from 10.6 have this problem.


Cheers,
Witek

The mapOfMatPropVects with ref key still changes Geant4 behavior.  

For example a user could use the persisted GDML as a base geometry from which to vary the properties of one bordersurface or skinsurface in some performance study.  
With the map in place they will be surprised to find they are changing all the surfaces at that use the matrix at once whereas they wanted to change just 
the properties of one surface.
Actually it is probable that most users would not notice this and 
would get erroneous results.

For me simplicity is elegance. The simpler you can do things the better because it makes it easier you to maintain and easier for users to understand. As Geant4 is a framework with a hugely diverse user base clearly minimizing assumptions and not changing behaviour in a way that will break the usage of some users is highly favored.

This is especially the case here where I can see no credible benefit for this map. To me the map is a bug that should be swiftly eliminated.

The current map is clearly a bug and needs to be fixed, but according to my current understanding I don't fully agree with your argument against using 'ref' as a key.

If a user loads GDML where one matrix (one 'ref') is reused in the properties of several surfaces, a natural thing would be that the same C++ vector object is then reused in Geant4 geometry in memory for those surfaces.
I mean, if several surfaces are bound to the same properties vector in GDML, that binding should be also reproduced in Geant4 geometry in memory, don't you agree? 
The GDML reader should perform a 1 to 1 mapping of what is in GDML file into the C++ geometry.
If a user wants to independently change properties of different surfaces, he should use separate matrices at the level of GDML, which will then be translated into separate C++ objects in memory.

Turning your example around, if we don't use the map with 'ref' as the key and a user implements several surfaces in GDML reusing the same matrix, he may be surprised that at the level of the C++ geometry, while changing the properties of one surface, the other ones stay unaffected. I mean, he may expect to have the same G4MaterialPropertyVector objects shared by those surfaces (as in the GDML file), which wouldn't be the case if we don't use the map.

I agree with you, that either way, there is some assumption about the GDML->C++ translation to be taken into account, but for me, reproducing the same couplings between the entities in GDML and the objects in C++ would be the most natural way.

You are changing behaviour, adding complexity and not getting any benefits.

The mental model of the majority of users is in terms of surfaces and 
their properties. They are mostly unaware of the implementation details
of the GDML and they certainly do not think in those terms.
The hierarchy of models is as below with each level getting more subservient
and more of an implementation detail.

1. mental model 
2. C++ model
3. GDML model 

Adding the map ties together the properties of multiple surfaces in a 
way that users will not expect. It imposes an assumption that
does not conform to the natural way of thinking about surfaces and their properties.
  
It breaks the rule of not surprising your users. 

Many users will be building their geometry in C++ and using the GDML just as a convenient way to persist it. So they do not regard the GDML as the source of the geometry, for them it is an implementation detail that they should not need to be very familiar with.  

Also, consider how you would want to document the change in behaviour from 
adding the map ? Why would you bother to confuse your users with such a thing, without any discernable benefit ? 

Keep things simple, fix the bug and move on.

I don't understand your statement: 

"Adding the map ties together the properties of multiple surfaces in a 
way that users will not expect."

Case 1: no map

a) if in GDLM several surfaces use different property vectors -> this gets translated to those surfaces using different property vectors in C++, so OK

b) if in GDML several surfaces use the same property vector -> this gets translated to those surfaces still using DIFFERENT property vectors in C++, so you lose the binding that was (maybe on purpose) introduced in GDML, for me this is also a bug

Case 2: map using 'ref' as key

a) if in GDLM several surfaces use different property vectors -> this gets translated to those surfaces using different property vectors in C++, so everything OK, the map does not tie anything together

b) if in GDML several surfaces use the same property vector -> this gets translated to those surfaces using the same property vector in C++, so the binding present in GDML is preserved in C++, for me this is the expected behavior.

If you implement your geometry in C++ and write out the GDML, the different surfaces will have different properties vectors in GDML (different refs) and when you read it back, the presence of the map will not tie anything together (you will be in case 2 a). You will be able to do the full cycle C++ -> GDML -> C++ without adding any ties.

Anyway, if people think that not having the map leads to a more 'natural' behaviour, I am fine with removing it. Let me check with the other Geant4 developers so see what they think.

Understandably you have the GDML-centric viewpoint of a GDML expert. Users do not think like that, they have surfaces with properties and they do not care how that is represented in GDML and they definitely do not want to have to study the GDML implementation to understand how to change the values of surfaces properties without getting nasty surprises.

I didn't look at the insights of the implementation, but by reading the thread in this ticket, it seems to me Witek's assumptions are right, in the sense that the C++/GDML binding would be properly preserved in all cases (adoption of same or different property vectors). It also appears to me the most straightforward fix, based on the previous introduction and adoption of the map...
On the other side, if I understand well, Simon's main concern is about backwards compatibility of existing GDML files, which through this approach could be [silently] broken... is this really the case? Would the import of existing GDML files be broken or misbehaving with such fix?  This is not clear to me.
To note that the same concept, in a sense (and for a very good reason) applies to the binding of solids and volumes (or materials and volumes): you may have many volumes that share the same solid (or material), but not for this solids (or materials) are replicated in the persistent GDML description; if, in GDML, one modifies the specification of a solid (or a material), this will modify all volumes associated to it. I honestly don't see this a surprise effect for a user, at least, not for a user that apply changes in a not "naive" way.

The analogy with solids and volumes is not valid in my opinion. Any user that wants to load GDML and then change solids should expect to change all solids because that is the nature of the geometry model. That is different to the nature of the API for surfaces and their properties in my opinion. Users (at least this user) thinks that the properties of a surface should be able to be modified independently without being surprised by that change magically changing the properties of other surfaces.
Remember that the user does not know or care or want to how about the GDML implementation.

It is difficult to debug future code in comments. Thus I suggest that you should proceed with the map using the ref key if you wish and then I will try to exercise
it looking for surprising issues once I get around to looking at 1071(?).

The point in this is to preserve as much as possible the binding with the C++ implementation in GDML. So the reasoning which applies for volumes vs solids/materials is exactly valid also for the optical surfaces. This will lead to a more compact description in the GDML file, as well as reduced memory footprint in the final memory representation. Opinions of users are not always correct and sometimes only partially considering all the aspects associated to an implementation choice.

Fix is now included in the development branch and will be included in the next patch release.