Some final input on the IVC OO update #59

gonuke · 2024-03-27T03:53:41Z

Here are a number of changes to the latest update to streamline a number of things. I used a number of small commits to show the individual changes, mostly distinct from each other. NOTE: this PR will update the ivc_oo branch thus changing PR #55

None of this has been tested since I don't have a robust testing environment for the whole process, so there may be some typo bugs.

use a more conventional singleton pattern for cubit initialization
move the defaults to be members of utils rather than returned by a function
don't overuse try/raise blocks
make IVB.Surfaces and IVB.Components into dictionaries based on their name since we need this info together often
put module imports in a more pythonic order - native modules to third party modules to custom dependencies to local modules (hope this doesn't break anything!)
import cubit_io as module and qualify method names
separation of concerns in parastell: construction vs export (with docstrings)
tweak variable names for consistency
refactor to avoid making the component dictionary that requires special cases between magnets and otherwise

connoramoreno

Thanks @gonuke, I think a lot of these changes make things more streamlined than before.

I just have some comments/questions involving structure and syntax. For the smaller changes involving syntax, I'm happy to take care of them myself.

connoramoreno · 2024-03-27T15:47:44Z

src/cubit_io.py

-        cubit_initialized = True
-
-    return cubit_initialized
+        initialized = True


The global variable initialized will not be updated outside this function. To have the update apply globally, initialized either needs to be returned or be referenced as a global variable; see corresponding suggestion

connoramoreno · 2024-03-27T15:50:04Z

src/cubit_io.py

    """
-    if not cubit_initialized:
+    if not initialized:


Suggested change

if not initialized:

global initialized

if not initialized:

hopefully that is enough to handle all the other assumptions I make about how this should work

connoramoreno · 2024-03-27T15:52:54Z

src/cubit_io.py

@@ -61,6 +53,8 @@ def export_step_cubit(filename, export_dir=''):
        export_dir (str): directory to which to export the STEP output file
            (defaults to empty string).
    """
+    init_cubit()


If STEP files are being exported, presumably Cubit has already been initialized? Is there a different reason for calling init_cubit()?

Maybe it should also be mentioned that with newer versions of Cubit (perhaps this was true for older versions as well), calling Cubit functions in Python without explicitly initializing Cubit beforehand will initialize Cubit with default parameters. We define our own Cubit initialization to limit output and set the Svalinn plug-in directory (which is only relevant for the legacy DAGMC export).

The way we have written it (possibly with your corrections), this function costs basically nothing after the first time. The reason to call it often is just to be robust against other people using these methods in ways we don't expect. An alternative would be to check initialization and raise an exception. (Or maybe that would happen anyway if not initialized?)

Fair enough! I think calling init_cubit at the beginning of each function as you've implemented is simpler than checking initialization. My only concern has to do with the behavior of Cubit re-initialization.

Is it guaranteed that re-initializing effectively doesn't do anything? After testing I can guarantee that it doesn't affect geometry but Cubit's Python source code isn't transparent enough for me to see anything else that might be going on.

I agree that some of the additions I made are probably a little superfluous, but since they are essentially free, it seems like it's ok.

Agreed that it's okay, and since we've confirmed that the state of initialized persists, calling init_cubit in each function is harmless

connoramoreno · 2024-03-27T15:55:02Z

src/cubit_io.py

@@ -74,6 +68,8 @@ def export_mesh_cubit(filename, export_dir=''):
        export_dir (str): directory to which to export the H5M output file
            (defaults to empty string).
    """
+    init_cubit()


Similar question as above about initialization.

Using our Cubit initialization function does feel more necessary here to set the Svalinn plug-in path, though only if this function is called outside ParaStell.

connoramoreno · 2024-03-27T15:55:16Z

src/cubit_io.py

@@ -101,6 +97,8 @@ def export_dagmc_cubit_legacy(
        export_dir (str): directory to which to export the DAGMC output file
            (defaults to empty string).
    """
+    init_cubit()


Similar question as above about initialization.

connoramoreno · 2024-03-27T16:22:55Z

src/invessel_build.py

+    def merge_layer_surfaces(self):
+        """Merges ParaStell in-vessel component surfaces in Coreform Cubit based on
+        surface IDs rather than imprinting and merging all. Assumes that the
+        radial_build dictionary is ordered radially outward. Note that overlaps
+        between magnet volumes and in-vessel components will not be merged in this
+        workflow.
+        """
+        # Tracks the surface id of the outer surface of the previous layer
+        prev_outer_surface_id = None
+
+        for data in self.invessel_build.radial_build.values():
+
+            inner_surface_id, outer_surface_id = (
+                orient_spline_surfaces(data['vol_id'])
+            )
+
+            # Conditionally skip merging (first iteration only)
+            if prev_outer_surface_id is None:
+                prev_outer_surface_id = outer_surface_id
+            else:
+                cubit.cmd(
+                    f'merge surface {inner_surface_id} {prev_outer_surface_id}'
+                )
+                prev_outer_surface_id = outer_surface_id
+


It should be noted that unless _import_ivb_step in parastell.py is called, the radial_build dictionary will not have vol_id keys in its subdictionaries, which is why I originally had it as a non-member function that used a separate dictionary input.

Upon reflection, though, I think this is better since that failure mechanism is built-in. We should maybe add some error block that captures that failure though.

I was on the fence about making this a member function. In the end, I thought this was cleaner. On one hand, it's true that you need to have run _import_ivb_step(), but you also need to have an InvesselBuild object to do this, so it makes sense to be a member function.

connoramoreno · 2024-03-27T16:31:12Z

src/parastell.py

+from src.utils import invessel_build_def, magnets_def, source_def,
    dagmc_export_def


Since these imports are on separate lines, we'll need some way to make Python recognize that they are all from the same script, such as parentheses surrounding the imports or a \ at the line break

src/parastell.py

connoramoreno · 2024-03-27T16:44:28Z

src/parastell.py

@@ -131,6 +145,14 @@ def construct_invessel_build(self, invessel_build):
        self.invessel_build.populate_surfaces()
        self.invessel_build.calculate_loci()
        self.invessel_build.generate_components()
+
+    def export_invessel_build(self, ivb_dict):


For each of these export methods, should the corresponding dictionary be stored as a member variable in the corresponding construction method so that users don't need to add it again as an argument?

Alternatively, we could remove the export-related variables from the corresponding dictionaries and use them as arguments in the export functions instead.

I thought about doing the latter, but it is less robust against making changes to the content of the dictionary.

I'm comfortable with leaving it as is since the construct methods and export methods then have similar signatures and I think it makes it pretty easy to follow.

connoramoreno · 2024-03-27T16:51:26Z

src/parastell.py

-        ivb_dict = invessel_build_def.copy()
-        ivb_dict.update(invessel_build)


Without incorporating the default dictionaries in the construction methods, there will be no default parameters defined for the input dictionaries should the user choose to interface with ParaStell via Python rather than YAML. Users would need to define every parameter themselves.

Couldn't they do it the same way we do it here? Load the defaults from utils and then overwrite as they choose?

Certainly! Just a tad less streamlined from a UI perspective.

gonuke · 2024-03-27T18:42:01Z

I think I addressed the bugs that were noted, and don't think other changes are necessary.

connoramoreno

Going through the tests, there were several small issues regarding syntax. I also have a question regarding the incorporation of the default DAGMC export dictionary (see below).

Can confirm that with suggested changes, all tests pass.

connoramoreno · 2024-03-27T19:41:25Z

tests/test_invessel_build.py

+import read_vmec
+
+import src.invessel_build as ivb


Importing in this order leads to a library linking issue for Cubit since Cubit ends up getting imported after read_vmec.

It's not pythonic but to fix, src.invessel_build can be imported before read_vmec. Note that nothing needs to be changed in the actual invessel_build script.

src/parastell.py

connoramoreno · 2024-03-27T20:09:55Z

tests/test_parastell.py

Since default dictionaries are no longer applied to Python input, need to specify all input parameters in corresponding dictionaries.

For invessel_build, need to add 'scale', 'export_cad_to_dagmc', 'plasma_mat_tag', 'sol_mat_tag', 'dagmc_filename', and 'export_dir' keys.

For magnets, need to add 'scale', 'step_filename', 'mat_tag', 'mesh_filename', and 'export_dir' keys.

For source, need to add 'scale', 'filename', and 'export_dir' keys.

Alternatively, you can import the default dictionaries from utils.py and modify them or copy and update the dictionaries as is done in the parastell() function of parastell.py

For some reason, I couldn't comment on specific lines of this file (presumably because they weren't changed) so I commented on the file itself.

Is there a particular reason for using the default dictionaries in the parastell() function rather than the original implementation?

As someone who will likely be using ParaStell via the Python interface rather than YAML in order to use NumPy operations and other math to define the thickness matrices, it would be nice to have the default dictionaries apply there as well to simplify input.

By implementing these changes to the default dictionaries, there is loss of functionality for Python users, whereas in the original way, the same functionality applies for both Python and YAML users.

connoramoreno · 2024-03-27T20:15:46Z

src/parastell.py

        export_dict = dagmc_export_def.copy()
        export_dict.update(dagmc_export)


Any particular reason that the default DAGMC export dictionary is incorporated here and not in the parastell() function like the other default dictionaries?

tests/test_parastell.py

Co-authored-by: connoramoreno <[email protected]>

connoramoreno · 2024-03-28T13:25:28Z

Thanks for the changes @gonuke. Now, we just need either the changes to the input dictionaries for test_parastell.py or we need to revert the implementation of default input dictionaries to how it is in #55

gonuke · 2024-03-28T16:24:36Z

In hindsight, I should have had you just merge the PR and then make additional commits on the original branch :)

connoramoreno · 2024-03-28T16:28:02Z

Thanks @gonuke

Some final input on the IVC OO update

gonuke added 11 commits March 26, 2024 21:58

singleton pattern for initialization

44a6394

simply treatment of defaults

afdb1f6

Directly test for known failure conditions rather than try/raise

6f84bfa

convert Surfaces and Components to dicts for simplicity

06bca3c

reorder imports based on native to local ordering

754b983

tidy up use of cubit_io

8049752

separation of concerns for IVB

6ed88be

separation of concerns: construction vs export

f2ee156

add docstrings

59c8c41

more consistent variable name

7855700

eliminate need to construct another component dictionary

442f9a1

gonuke requested a review from connoramoreno March 27, 2024 03:53

gonuke added 2 commits March 27, 2024 00:02

Update tests with some cleanup

43d4e5c

remove redundant == True

9b26758

connoramoreno requested changes Mar 27, 2024

View reviewed changes

gonuke added 2 commits March 27, 2024 14:40

globalize initializatoin state

6eb63c5

group multi-line import

2cb1eda

connoramoreno requested changes Mar 27, 2024

View reviewed changes

gonuke and others added 2 commits March 27, 2024 21:49

review suggestion

0097760

Co-authored-by: connoramoreno <[email protected]>

review suggestion

4b05b53

Co-authored-by: connoramoreno <[email protected]>

gonuke added 2 commits March 28, 2024 11:23

consistent use of defaults

d1e2f4c

fix import ordering

aa97b68

connoramoreno merged commit e34a0f8 into svalinn:ivc_oo Mar 28, 2024

connoramoreno added a commit that referenced this pull request Sep 10, 2024

Merge pull request #59 from gonuke/ivc_oo_pphw

dbfb24c

Some final input on the IVC OO update

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some final input on the IVC OO update #59

Some final input on the IVC OO update #59

gonuke commented Mar 27, 2024 •

edited

Loading

connoramoreno left a comment

connoramoreno Mar 27, 2024 •

edited

Loading

connoramoreno Mar 27, 2024

gonuke Mar 27, 2024

connoramoreno Mar 27, 2024

connoramoreno Mar 27, 2024

gonuke Mar 27, 2024

connoramoreno Mar 27, 2024

gonuke Mar 27, 2024

connoramoreno Mar 27, 2024

connoramoreno Mar 27, 2024

connoramoreno Mar 27, 2024 •

edited

Loading

connoramoreno Mar 27, 2024

connoramoreno Mar 27, 2024

gonuke Mar 27, 2024

connoramoreno Mar 27, 2024

connoramoreno Mar 27, 2024

gonuke Mar 27, 2024

connoramoreno Mar 27, 2024 •

edited

Loading

gonuke Mar 27, 2024

connoramoreno Mar 27, 2024

gonuke commented Mar 27, 2024

connoramoreno left a comment

connoramoreno Mar 27, 2024

connoramoreno Mar 27, 2024 •

edited

Loading

connoramoreno Mar 27, 2024

connoramoreno Mar 27, 2024

connoramoreno Mar 27, 2024

connoramoreno commented Mar 28, 2024

gonuke commented Mar 28, 2024

connoramoreno commented Mar 28, 2024

		from src.utils import invessel_build_def, magnets_def, source_def,
		dagmc_export_def

		ivb_dict = invessel_build_def.copy()
		ivb_dict.update(invessel_build)

		export_dict = dagmc_export_def.copy()
		export_dict.update(dagmc_export)

Some final input on the IVC OO update #59

Some final input on the IVC OO update #59

Conversation

gonuke commented Mar 27, 2024 • edited Loading

connoramoreno left a comment

Choose a reason for hiding this comment

connoramoreno Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connoramoreno Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connoramoreno Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gonuke commented Mar 27, 2024

connoramoreno left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connoramoreno Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connoramoreno commented Mar 28, 2024

gonuke commented Mar 28, 2024

connoramoreno commented Mar 28, 2024

gonuke commented Mar 27, 2024 •

edited

Loading

connoramoreno Mar 27, 2024 •

edited

Loading

connoramoreno Mar 27, 2024 •

edited

Loading

connoramoreno Mar 27, 2024 •

edited

Loading

connoramoreno Mar 27, 2024 •

edited

Loading