Add DAGSet.create classmethod

[paulromano]

This PR adds a DAGSet.create classmethod that can be used to create new volumes/surfaces that functions similar to the Group.create classmethod:

vol = dagmc.Volume.create(model, ...)
surf = dagmc.Surface.create(model, ...)

One thought I had regarding design is that it might be better to have create methods on the DAGModel class instead since the model always has to get passed, so that the above example would look like:

vol = model.create_volume(...)
surf = model.create_surface(...)

[gonuke]

Do we also imagine a way to populate these meshsets? What's the utility in generating new meshsets if we don't have a way to populate them with triangles? Or do we imagine that a pydagmc user/developer would have a way to do this and we just want to provide the set manipulation interface?

[paulromano]

Yeah, I hear you @gonuke. It may not seem that useful at first glance. To get a sense of how one could use this, see the following snippet (from a branch of mine for stellarmesh):

https://github.com/paulromano/stellarmesh/blob/028b4ff74e95592cc31e418364a2b3beb46bc216/src/stellarmesh/moab.py#L544-L555

After a surface is created, the actual triangles are loaded from an STL file that was written by gmsh.

[gonuke]

Thanks @paulromano - One question about the safety of setting the global_id - but maybe this should be an issue about that setter?

[ahnaf-tahmid-chowdhury]

I assume, we just need a rebase to merge this?

[ahnaf-tahmid-chowdhury]

I believe that adding some documentation comments would be very helpful for future improvements to our documentation. I'm willing to review all the code and create a PR to add the necessary documentation support once this is merged.

[pshriwise]

I believe that adding some documentation comments would be very helpful for future improvements to our documentation. I'm willing to review all the code and create a PR to add the necessary documentation support once this is merged.

@ahnaf-tahmid-chowdhury that would be welcome prior to the initial release. Can I let you know when the time comes? I don't want you to waste your time while we're still changing aspects of the design/API.

[pshriwise]

It seems we'll need to re-think the ID setting now that #17 has been merged.

I guess one question here is whether or not it's valid to return a DAGSet object rather than a dagmc.Volume or dagmc.Surface object (as you mentioned in the PR description). Combined with the ID assignment issue, maybe DAGModel.create_volume()/create_surface() methods are really the way to go.

[ahnaf-tahmid-chowdhury]

Can I let you know when the time comes?

I would be grateful if you tell me so. Maybe there was a community call planned for this that I might have missed.

[gonuke]

It seems we'll need to re-think the ID setting now that #17 has been merged.

I guess one question here is whether or not it's valid to return a DAGSet object rather than a dagmc.Volume or dagmc.Surface object (as you mentioned in the PR description). Combined with the ID assignment issue, maybe DAGModel.create_volume()/create_surface() methods are really the way to go.

I'll need to refresh my understanding of calling parent class methods, but I think the current design is mostly OK/makes sense. Either way, we'll need some way to do the work that @paulromano adds in DAGSet.create() and since every DAGSet needs to know which DAGModel it's in, we'll need to pass that anyway, regardless of where it lives.

I think each derived class can have it's own create() method that first calls the DAGSet.create() and then handles the geometry specific metadata initialization/assignment. If we rely on the DAGSset.id setter, we can safely manage the ID space. We could also modify this setter to automatically get a valid ID when there is an overlap, rather than raising an exception.

[pshriwise]

I'll need to refresh my understanding of calling parent class methods, but I think the current design is mostly OK/makes sense. Either way, we'll need some way to do the work that @paulromano adds in DAGSet.create() and since every DAGSet needs to know which DAGModel it's in, we'll need to pass that anyway, regardless of where it lives.

Ah yeah, I was thinking the problem was that we didn't have the right type around to do the internal ID check properly. Moving the ID assignment later in the DAGSet.create method should work -- made a suggestion along these lines @paulromano. Added a suggestion along those lines. We definitely want to keep leaning on the DAGSet.id setter, sorry if that read otherwise.

It still feels a little awkward to me to be passing the DAGModel in and more intuitive to turn to the model object for the generation of new geometry objects, but those can be added later based on this existing method pretty easily if desired and I don't think the existence of one precludes the other.

We could also modify this setter to automatically get a valid ID when there is an overlap, rather than raising an exception.
👍🏻

[gonuke]

See #21 for updates to the id.setter - still raises an exception on overlap. Looking for collective thoughts on what the right choice is here. I think @pshriwise gave a 👍

[paulromano]

@pshriwise Sorry for the delay on updating this -- hopefully should be good to go now. One thing I noticed while working through this is that there were two test_volume functions, so the first one was effectively not getting tested. Once I fixed that, it revealed some small issues that I've also resolved.

[pshriwise]

@pshriwise Sorry for the delay on updating this -- hopefully should be good to go now. One thing I noticed while working through this is that there were two test_volume functions, so the first one was effectively not getting tested. Once I fixed that, it revealed some small issues that I've also resolved.

Thanks @paulromano! And nice catch. It seems we were writing too many tests! *jokes*

Also, I'm not sure who's manufacturing this irradiated olive oil, but I'm going to avoid that brand...