Fix for finding redundant surfaces

[eepeterson]

I was noticing some models I was generating weren't finding redundant surfaces (particularly Z cones) because some of the coefficients differed in the last few decimal places.

[pshriwise]

This looks like a nice update to me @eepeterson.

General question about this (but also related to the default behavior), if someone were to specify a SurfaceFilter, would that filter still be valid if the surface is redundant and removed when the geometry is exported?

[eepeterson]

No I don't think so actually. We could do it through the Model class on export to check that, but I don't see a particularly good way to link the tallies and geometry otherwise.

[eepeterson]

I'll raise that as an issue so we don't lose track of it, but is there anything else you'd want to see in this PR or are you fine with a hard-code 10 decimal precision rounding?

[pshriwise]

I'll raise that as an issue so we don't lose track of it, but is there anything else you'd want to see in this PR or are you fine with a hard-code 10 decimal precision rounding?

@eepeterson how would you feel about a default of rounding to the 10th digit, but making it a parameter of Geometry.get_redundant_surfaces? Even though the chance of someone wanting differentiate surfaces with higher precision is small, it would make it possible without a change to the codebase itself.

[paulromano]

Agree with @pshriwise -- it would be nice to be able to turn that knob if need be

[eepeterson]

Sounds good. I'm inclined to also set that as an attribute on the Geometry class because if we only add it to the parameters of Geometry.get_redundant_surfaces then we will need to add it to the parameters of export_to_xml for both Model and Geometry which gets a little sloppy. I can add it as an optional parameter to Geometry.get_redundant_surfaces, but also if we add attributes like Geometry.remove_redundant_surfaces = True and Geometry.surface_precision = 10 then we could remove the remove_surfs kwarg from export_to_xml which makes things a bit cleaner I think. It would be a breaking change that we can warn about, but I'm inclined to go in that direction. It would also be great if we could default to removing redundant surfaces since I have only encountered them causing tracking problems and can't see a good reason for keeping them in the model, but it does screw up all the input files in the test suite. Thoughts?

[pshriwise]

This is a nice improvement! Adding a couple of thoughts for you to mull over, @eepeterson.

[pshriwise]

Probably splitting hairs here, but how would you feel about a name like merge_surfaces?

[pshriwise]

My preference would be to set the attributes rather than expose them here.

[pshriwise]

As far as the default value of these attributes goes, I'm okay with moving to a default value of True for removing redundant surfaces. In a separate PR, we may want to first figure out an elegant way to update other objects that may rely on surfaces that get removed on export. I'm thinking that we might optionally have Geometry.remove_redundant_surfaces return a mapping of removed surfaces to new surfaces that could be used to update tallies automatically. It might look something like:

surf_map =  geometry.remove_redundant_surfaces(return_mapping=True)
tally.update_surfaces(surf_map)

[eepeterson]

Thanks for the comments @pshriwise! I'll get on those immediately. Would the get_redundant_surfaces method suffice for that mapping? I think it's exactly what you're asking for, no? I can work up a PR to update the surfaces in a Tally and make it automatic if both the Geometry and Tallies are within a Model container. I'll also plan to move to Geometry.merge_surfaces = True in that PR as well.

[pshriwise]

Would the get_redundant_surfaces method suffice for that mapping? I think it's exactly what you're asking for, no?

Hah, it sure is! Duh.

So I suppose one could call get_redundant_surfaces separately and pass it to a new Tally.update_surfaces method. If one were then to remove the redundant surfaces, that map would be recomputed.

Or we could optionally return it from remove_redundant_surfaces. I generally like returns from methods/functions to be consistent, but this seems like a case where we might want to allow it.

Also, is that mapping deterministic? I'm guessing that it is since it'll be based on which surfaces are visited first in the ordered dictionary of cells returned from get_all_cells.

[eepeterson]

So I suppose one could call get_redundant_surfaces separately and pass it to a new Tally.update_surfaces method. If one were then to remove the redundant surfaces, that map would be recomputed.

I was thinking of maybe caching the redundant surface map when remove_redundant_surfaces is called for this reason. I do like the idea of get_redundant_surfaces returning an empty map if they have already been removed and to me the name remove_redundant_surfaces doesn't imply you're getting anything back, which is why I would opt to keep the methods as they are, with potentially an added attribute (public or private) for the cached redundant surfaces. I also do think the mapping is deterministic for the reasons you stated.

[pshriwise]

I was thinking of maybe caching the redundant surface map when remove_redundant_surfaces is called for this reason. I do like the idea of get_redundant_surfaces returning an empty map if they have already been removed and to me the name remove_redundant_surfaces doesn't imply you're getting anything back, which is why I would opt to keep the methods as they are, with potentially an added attribute (public or private) for the cached redundant surfaces.

Ah yeah good idea! What would you imagine the workflow for updating tally information looks like in this case? In my head this might be:

geometry.remove_redundant_surfaces() # creates cache with mapping
tally.update_surfaces(geometry.surface_mapping_cache) # cache name TBD

I think this would be fine but it requires users to know about this cache. And, if I'm understanding correctly, they would get an empty map if they tried to retrieve a mapping from get_redundant_surfaces in the second line.

What about caching in get_redundant_surfaces instead? This feels more natural to me. Thoughts?

mapping = geometry.get_redundant_surfaces() # cache map here
tally.update_surfaces(mapping)
geometry.remove_redundant_surfaces() # use cached map and set cache to `None` at the end

[eepeterson]

I think I got what you suggested implemented @pshriwise

[pshriwise]

Looks good to me @eepeterson! I'll merge this tomorrow if there are no further comments from others.

[paulromano]

Sorry to chime in somewhat late here. Looking through the changes, I'm not super thrilled with the design and wondering if the following would simplify things:

• Let the user be responsible for calling remove_redundant_surfaces. That is, export_to_xml will not remove redundant surfaces for you -- it will simply export the model to XML. To me, it makes more sense that exporting should not modify the state of the geometry/model.
• The remove_redundant_surfaces method could return the same dict given to it by get_redundant_surfaces for the case where a user needs to modify related data structures (like tallies).
• surface_precision would only be an argument of remove_redundant_surfaces
• Geometry then wouldn't need any of the three extra attributes

[eepeterson]

@paulromano thanks for the feedback! I agree your approach simplifies things, but it misses the two main points of the PR I was hoping to achieve. 1.) I don’t see any good reason to allow writing duplicate surfaces to xml (certainly not by default). And 2.) I would prefer the user not have to explicitly ask the geometry to take care of that. Do you have reasons why that shouldn’t be the goal of this PR or is there a way we could get there with some of your suggested simplifications?

[paulromano]

I agree with your goals (cleanly avoiding redundant surfaces) but I'm just hesitant about adding state to the Geometry object that it is really only used in one or two methods. Some further ideas:

1. export_to_xml could produce a warning if there are redundant surfaces and advise the user to run remove_redunant_surfances()
2. We could have the C++ side be responsible for removing redundant surfaces and reassigning IDs
3. We could override __new__ for some of the surface classes to detect whether an instance of the same class exists with indistinguishable coefficients and if so just return it.

Personally, I think option 1 is simple enough and easy to implement. I also like that this gives the user an indication that maybe they are doing something that is not so wise in the first place. If we autoremove all redundant surfaces, it allows users to be a bit sloppier.

[eepeterson]

Sorry for the thrashing on this PR... I'll squash the commits once we agree on a strategy to keep the history a bit cleaner. @paulromano I do like your first suggestion the most and it's actually most similar to the original implementation in this PR. Some of the nitpicks I have with the current design are below:

1. The warning issued in export_to_xml calls get_redundant_surfaces with no ability to pass precision in, so the test for redundancies will not necessarily be the same as when the surfaces are removed. I think you'd agree adding arguments to export_to_xml is not the way to go, but would have to be if we don't add an attribute.
2. The check for redundancies before export is the only place outside the remove_redundant_surfaces method that we use get_redundant_surfaces so if it weren't for that we could put the logic of get_redundant_surfaces into remove_redundant_surfaces and drop that method altogether. Also, if we're going to check for them when exporting we might as well remove them in my opinion.
3. It still requires the user to remove surfaces manually.

I think I disagree with your comment on allowing users to be sloppier. While that's one way to look at it, I think heavy use of CompositeSurfaces which are very helpful for building complex parameterized models without tracking all the individual surfaces really necessitates this feature for every model. I guess what I'm trying to say is it's giving the user more power, not that they are now allowed to do something "wrong" or "bad" and get away with it.

With the addition of a couple attributes I think we benefit in a number of ways

1. Redundant surfaces can be removed automatically with no added arguments to export_to_xml
2. Making this behavior default is as simple as switching the default of one attribute
3. testing and removing redundant surfaces will now happen with the same precision
4. we could get rid of get_redundant_surfaces altogether
5. If we move towards a single model.xml input file it will be very easy for the Model class to automatically remove redundant surfaces and update any necessary tallies without any user involvement.

As for options 2 and 3 you suggested above: I would prefer to not have surfaces in the xml that get removed under the hood on the C++ side - I'd like the xml to be an accurate representation of the model. As for manipulating __new__ I do like the idea of never creating redundant surfaces in the first place, but for it to be a more elegant approach than adding a couple attributes I think I'd only want to implement it once on Surface rather than in multiple places. I will give this some more thought, but I don't know if it would be inviting a little more refactoring or have its own traps associated with it.

[paulromano]

@eepeterson All good points. I think you've won me over, so if you want to revert back to what you had in the first place, I'll take another pass at it. Sorry to lead you astray!

[eepeterson]

Thanks @paulromano I appreciate it! I think this is ready for another pass when you get a chance.

[paulromano]

Looks good this time, accounting for all the discussion 😄 Thanks for the improvement @eepeterson!