OO NWL

[gonuke]

Object-oriented version NWL geometry building capability. (comes after #99)

[connoramoreno]

Working through the OO version for NWL, I'm wondering if we actually want to limit users to creating either a full model or NWL geometry. It might make the code simpler to enforce that limit, but users might want to create both in the same run, rather than create each separately. Moreover, if creating each in separate runs, a source mesh would be generated twice.

[gonuke]

I think we should make each invocation of parastell separate, but we may want more variations on what folks can do when then call parastell. For example:
parastell --nwl (default=False) --ivb (default=True) --magnets (default=True) --source_mesh (default=True)
Then a user can mix & match?

[connoramoreno]

Actually, I really like that. Then, we wouldn't need a collection of executables; we'd only need a single parastell executable.

[gonuke]

I've just pushed an update with the latest kwargs PR merged.... This one probably fails tests because of missing functions

[gonuke]

Happy to hand it off to you now @connoramoreno

[connoramoreno]

Now that we have all command-line capability handled by parastell, do we need parse_args, etc. in invessel_build.py, magnet_coils.py, and source_mesh.py?

[gonuke]

Now that we have all command-line capability handled by parastell, do we need parse_args, etc. in invessel_build.py, magnet_coils.py, and source_mesh.py?

They may not be used much, but let's leave those for now. There may be a use case in the future 🤷

[connoramoreno]

I went through several renditions of incorporating NWL but I think I finally landed on something I like. I've made input for NWL geometry independent of IVB geometry (I had them coupled at first) but I think this is better, and simpler to implement from a coding perspective.

I also changed some command-line flags to use action=store_true since I couldn't get the default values to behave well with user input (it seems like user input was ignored in favor of the default value). It's entirely possible I was doing something wrong when calling the command-line executable but I think this version works well too.

Let me know if you'd rather I used default=False instead of action=store_true, though. An example call, as-is, to incorporate all geometry creation capability would be

parastell config.yaml -imsn

but, of course, users can choose which geometry to create via the corresponding flags.

[gonuke]

One of the goals of my previous PR was to separate the steps of building models from the step of exporting models. Maybe that matters less in the parastell script because it is not intended to be imported and used by others, but is a command-line interface itself.

[gonuke]

Probably going to need a brief in-person design discussion.

Why not take IVB & source mesh input and use them to construct an NWL dictionary that you use for NWL, thus avoiding the need for separate input?

Also, a carefully constructed NWL dictionary will look like an IVB dictionary and then reuse existing methods to build & export IVB?

[gonuke]

This radial_build is empty at this point.

[connoramoreno]

This conditional modifies radial_build to contain a nwl_geom component, getting rid of plasma and sol (if present), both of which get automatically added to radial_build.

An alternative workflow might be to have some flag for RadialBuild that indicates whether to add plasma and sol, defaulting to True. Then we could use the same construct_invessel_build method for both the in-vessel component and NWL geometries.

[connoramoreno]

If we implement the above workflow for RadialBuild, we wouldn't actually need to have explicit workflows for NWL geometry construction and export. Users could just construct and export a NWL geometry using the other tools.

For example, via the Python API:

... IVB parameters...
radial_build = {
    'nwl_geom': {
        'thickness_matrix': np.zeros(len(toroidal_angles), len(poloidal_angles)),
        'mat_tag': Vacuum
    }
}
nwl_geom = Stellarator(...)
nwl_geom.construct_invessel_build(
    ...
    radial_build,
    add_plasma_and_sol=False,
    ...
)
nwl_geom.export_invessel_build(export_cad_to_dagmc=False)
nwl_geom.export_dagmc(
    skip_imprint=True,
    filename='nwl_geom'
)

where the add_plasma_and_sol flag would default to True. Alternatively, via the command-line API

parastell config.yaml -i

where config.yaml has the appropriate geometry definition and flag shown for the Python API.

[gonuke]

... both of which get automatically added to radial_build.

Right! I forgot about the setter and how it automatically adds things!!!

[gonuke]

This seems identical to construct_invessel_build but with tweaked/customized parameters

[connoramoreno]

It's similar, but there's some extra work to change radial_build such that it contains only the nwl_geom component.

[gonuke]

Maybe I misunderstood our slack exchange, but I thought we were going to still try to streamline this to just use construct_invessel_build instead of a separate method?

[connoramoreno]

Maybe there was some miscommunication. Inside the parastell function, I can basically copy/paste what's in this method (and same for the export method) under the NWL conditional, or I can implement what I discussed in this comment.

[gonuke]

Is there a reason we can't/don't use cad_to_dagmc here?

[connoramoreno]

CAD-to-DAGMC does not store individual surface IDs in the DAGMC model, instead assigning a single ID to all surfaces in a volume. To tally surface crossings appropriately for NWL, we need to be able to isolate the spline surface.

[connoramoreno]

One of the goals of my previous PR was to separate the steps of building models from the step of exporting models. Maybe that matters less in the parastell script because it is not intended to be imported and used by others, but is a command-line interface itself.

Well I think separating construction from export as you had organizes the code better, but with the addition of geometry capabilities (IVB, magnets, etc.) being conditional on command-line flags, I thought organization might be better if separated by each condition.

An alternative that maintains separation between construction and export is to have conditionals inside "full model" construction and export functions, passing around args. This would be quick to implement if you think it's better.

[gonuke]

Is this ready for a final review @connoramoreno ?

[connoramoreno]

Yes, I'd say so!

[gonuke]

If we're able to streamline away from a special NWL build, can we also streamline away from a special NWL export?

[gonuke]

Because I'm the original author here, we may have ourselves in a position where I can't formally review this...

[connoramoreno]

Are you comfortable with that? Otherwise, I can open a new PR

[connoramoreno]

I decided to incorporate a flag for whether the interior vacuum chamber (plasma and, if present, SOL volumes) gets split into those two components or is merged into a single volume (such as for NWL geometry). I realized this is a functionality that is useful even outside of NWL geometry creation to reduce the number of volumes and surfaces of the overall model (if they are both the same material definition, such as void, it is safe to have them merged into a single volume).

[gonuke]

the handling of the NWL case still seems more complicated than it needs to be to me....

[gonuke]

Don't we always have this attribute now?

Also, is this construct typical? It seems confusing to me since it looks like it just sets the variable equal to itself. I know it is calling the setter, and therefore doing other things. I wonder if we move the actual code of the setter into some other function (update_separate_chamber or something) and call it form both the setter and directly here?

[connoramoreno]

I'm not sure how typical it is. However, I agree that having a function for this would clarify what's going on, rather than just calling the setter.

[connoramoreno]

While true that the class will always have the separate_chamber attribute once initialization completes, the hasattr condition is there for the purpose of checking whether or not the attribute has been set yet. If the class is being initialized, separate_chamber will not yet have been set, and so I don't want the code to call the setter prematurely.

However, if wall_s is switched after initialization, I want the code to update sol in radial_build accordingly, which is handled by the separate_chamber setter.

[gonuke]

This method does a lot, but I'm not entirely sure the use case. It looks like it may be set up to switch between true/false values of separate_chamber but that seems like an unlike case to me. If we don't ever allow that to happen, is all of this still necessary?

[connoramoreno]

Yes, much of the architecture here is to make the class robust against switching the value after initialization. If we assume the user won't try to do so, or put in a protection against it, much of the code can be removed. The same goes for some of the other setters.

[connoramoreno]

I've committed some of the above suggestions, but have held off on making any changes to the architecture protecting against attribute changes after class initialization until we decide how we want to proceed.