split MagnetSet and MagnetCoil #182

Edgar-21 · 2024-12-27T19:10:34Z

Round 2 at splitting this class to add the custom geometry functionality while preserving all the other features. After being away from this for a bit I realized I wanted to do it differently than in #173 and it was easier just to start fresh.

MagnetSet() is the parent class, and MagnetSetFromFilaments and MagnetSetFromGeometry inherit from it. MagnetSetFromFilaments is functionally the same as the current MagnetSet class. MagnetSetFromGeometry enables use of existing CAD files with the typical parastell workflows. Both classes have tested to work with DAGMC model generation and with the radial distance utilities.

MagnetSetFromGeometry has no need for MagnetCoil objects, so a new object, Filament, has been introduced which contains the functionality needed for radial distance finding with both MagnetSetFromFilaments and MagnetSetFromGeometry.

Kind of a big diff but to get the tests passing many little things needed to be tweaked. There is very little new code here, mostly just moving around existing pieces.

Addresses #174

connoramoreno

Here are some initial thoughts - we can discuss these further if you disagree. This branch will also need to be rebased onto main.

connoramoreno · 2025-01-15T22:34:13Z

parastell/magnet_coils.py


-        self.volume_ids = list(range(first_vol_id, last_vol_id + 1))
+    Arguments:
+        coils_file (str): path to coil filament data file.


The reason for including coils_file as an argument for MagnetSetFromGeometry is that it is helpful when used in conjunction with the radial distance utility. It may be better just to modify the radial distance utility or have users create a custom workflow using the radial distance functions, since coils_file isn't used for any of the methods of MagnetSetFromGeometry.

I agree, I'll tackle updating rdu to work with both subclasses in an upcoming PR, I've created an issue and assigned myself here #186

connoramoreno · 2025-01-15T22:36:46Z

parastell/magnet_coils.py

+        geometry_file (str): filename of the existing coil geometry. Can be of
+            the types supported by cubit_io.import_geom_to_cubit()
+        working_dir (str): path to directory in which existing geometry
+            is saved.


Would it make sense to combine these two arguments? Just a relative path can be supplied.

yeah seems like a good plan

connoramoreno · 2025-01-15T22:38:26Z

parastell/magnet_coils.py

+        start_line (int): starting line index for data in filament data file
+            (defaults to 3).
+        sample_mod (int): sampling modifier for filament points (defaults to
+            1). For a user-defined value n, every nth point will be sampled.
+        scale (float): a scaling factor between the units of the point-locus
+            data and [cm] (defaults to m2cm = 100).


We can probably remove these optional arguments. If we want to include the ability to scale the geometry still, we should instead have a method to rescale.

yes, with removing the filament data from this subclass that makes sense

connoramoreno · 2025-01-15T22:42:53Z

parastell/magnet_coils.py

@@ -18,10 +18,6 @@ class MagnetSet(object):

    Arguments:
        coils_file (str): path to coil filament data file.


See comments on MagnetSetFromGeometry class about removing this as an argument. Same goes for the optional attributes.

connoramoreno · 2025-01-15T22:53:17Z

parastell/magnet_coils.py

+    def _instantiate_filaments(self):
+        """Extracts filament coordinate data from input data file and
+        instantiates Filament class objects.
+        (Internal function not intended to be called externally)
+        """


This function and those below that have to do with coil data/filament objects should be moved to the MagnetSetFromFilaments class

connoramoreno · 2025-01-16T16:47:58Z

parastell/magnet_coils.py

    def _cut_magnets(self):
        """Cuts the magnets at the planes defining the toriodal extent.
        (Internal function not intended to be called externally)


Would this not also be useful for imported geometries?

I'm assuming that people who bring their own cad will have it in state where it matches their geometry, or will have the means to do that.

connoramoreno

Just a quick thing on Cubit syntax for newer versions. We can (potentially) submit an additional review after Friday's meeting.

connoramoreno · 2025-01-22T18:56:57Z

parastell/cubit_io.py

@@ -155,9 +172,27 @@ def export_dagmc_cubit(
    cubit.cmd("mesh surface all")

    export_path = Path(export_dir) / Path(filename).with_suffix(".h5m")
-    cubit.cmd(f'export dagmc "{export_path}" overwrite')
+    cubit.cmd(f'export cf_dagmc "{export_path}" overwrite')


In more recent versions of Cubit, such as 2024.8+ which we now support, DAGMC export syntax was changed. Native cubit DAGMC faceting now has the syntax export dagmc

I suppose I can upgrade, this has now been reverted

parastell/magnet_coils.py

connoramoreno · 2025-01-24T22:56:42Z

@gonuke, Edgar and I discussed the software design of the code changes in magnet_coils.py and we'd like your input on best practices/pythonicity. I'm mostly on board with the changes Edgar is introducing here but I'm hesitant to approve this PR with the current implementation of the parent class, MagnetSet. Specifically, MagnetSet is not functional as a stand-alone class. Although it's a parent class, it relies on member variables, including self.geometry_file and self.working_dir in the import_geom_cubit method, being set by one of the child classes. Is that okay? I'm unsure what "proper" code design is here.

gonuke · 2025-01-24T23:15:10Z

@gonuke, Edgar and I discussed the software design of the code changes in magnet_coils.py and we'd like your input on best practices/pythonicity. I'm mostly on board with the changes Edgar is introducing here but I'm hesitant to approve this PR with the current implementation of the parent class, MagnetSet. Specifically, MagnetSet is not functional as a stand-alone class. Although it's a parent class, it relies on member variables, including self.geometry_file and self.working_dir in the import_geom_cubit method, being set by one of the child classes. Is that okay? I'm unsure what "proper" code design is here.

In general, this is a common pattern. For example, in C++, it's common to have a parent class that defines a common API (i.e. a set of functions) but define those function in the parent class to be explicitly undefined so that it's never legal to define an object from that parent class.

I'll take a closer look at this specific implementation to see if I have any thoughts, though.

connoramoreno

This is looking pretty good to me now, thanks @Edgar-21. I'll hold off on approving/merging until Paul gives his thoughts on our software design discussion

connoramoreno · 2025-01-27T20:44:59Z

tests/test_magnet_coils.py

+files_to_remove = [
+    "magnet_set.step",
+    "magnet_mesh.exo",
+    "magnet_mesh.h5m",
+    "stellarator.log",
+    "step_import.log",
+    "single_coil.step",
+]
+

 def remove_files():
+    for file in files_to_remove:
+        if Path(file).exists():
+            Path.unlink(file)
+


I'd like to change all of the tests to use this routine. I think a separate PR would be most appropriate for that though.

gonuke

I think the code looks OK. It's a little hard to track all the changes because many of them are just a reorganization of existing code that are hard to compare because of how they have moved in the file. It seems OK to me.

I have one minor request on the organization of the

.gitignore

Edgar-21 · 2025-01-29T23:21:24Z

rebased, and made a few changes to get things working with the new cad to dagmc methods. I've added a method to InvesselBuild() and MagnetSet() that adds the geometry to a cad_to_dagmc instance, rather than getting the cad solids from each class and combining them in Stellarator.build_cad_to_dagmc_model()

connoramoreno

A comment about code structure, almost done!

parastell/parastell.py

connoramoreno

Looks great! Thanks for this upgrade @Edgar-21

connoramoreno · 2025-01-31T21:42:24Z

Just need @gonuke to approve before we can merge.

gonuke

Thanks @Edgar-21

Edgar-21 marked this pull request as ready for review December 27, 2024 20:24

connoramoreno requested changes Jan 16, 2025

View reviewed changes

Edgar-21 mentioned this pull request Jan 21, 2025

Modify Radial Distance Utils to Work With Both Sub Classes of MagnetSet() #186

Open

Edgar-21 force-pushed the magnets_from_custom_geometry branch from 70e10da to 32e8675 Compare January 21, 2025 15:46

Edgar-21 requested a review from connoramoreno January 21, 2025 15:58

connoramoreno requested changes Jan 22, 2025

View reviewed changes

Edgar-21 commented Jan 24, 2025

View reviewed changes

parastell/magnet_coils.py Outdated Show resolved Hide resolved

Edgar-21 force-pushed the magnets_from_custom_geometry branch from 32e8675 to def847b Compare January 27, 2025 18:10

Edgar-21 requested a review from connoramoreno January 27, 2025 18:12

connoramoreno reviewed Jan 27, 2025

View reviewed changes

gonuke requested changes Jan 28, 2025

View reviewed changes

.gitignore Show resolved Hide resolved

Edgar-21 added 11 commits January 29, 2025 12:43

split MagnetSet and MagnetCoil

eac600f

add necessary test file

1e78df1

tweak docstrings in magnet_coils.py

a6545f4

tweak docstrings in parastell.py

a479874

tweak docstrings in radial_distance_utils.py

6da86ac

dont ignore magnet geometry test file

6e00607

remove all trace of coils file from MagnetSetFromGeometry

b8e5be6

move sort_filaments_toroidally to MagnetSetFromFilaments

41357a3

group test files in gitignore

8a790f1

clean up after rebase

18b65b2

update to have magnets work with new cad to dagmc workflow

0fa2477

Edgar-21 force-pushed the magnets_from_custom_geometry branch from def847b to 0fa2477 Compare January 29, 2025 23:10

Edgar-21 added 2 commits January 29, 2025 17:16

add docstrings to new MagnetCoil method

289c4a7

remove unused import

16c542f

Edgar-21 requested review from gonuke and connoramoreno January 29, 2025 23:21

connoramoreno requested changes Jan 31, 2025

View reviewed changes

parastell/parastell.py Outdated Show resolved Hide resolved

add solids in stellarator

f81059f

Edgar-21 requested a review from connoramoreno January 31, 2025 21:13

update docstring of ivb.extract_solids_and_mat_tags

f53be7b

connoramoreno approved these changes Jan 31, 2025

View reviewed changes

gonuke approved these changes Jan 31, 2025

View reviewed changes

connoramoreno merged commit 552801c into main Jan 31, 2025
3 checks passed

Edgar-21 deleted the magnets_from_custom_geometry branch February 3, 2025 18:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

split MagnetSet and MagnetCoil #182

split MagnetSet and MagnetCoil #182

Edgar-21 commented Dec 27, 2024 •

edited

Loading

connoramoreno left a comment

connoramoreno Jan 15, 2025

Edgar-21 Jan 21, 2025

connoramoreno Jan 15, 2025

Edgar-21 Jan 21, 2025

connoramoreno Jan 15, 2025

Edgar-21 Jan 21, 2025

connoramoreno Jan 15, 2025

connoramoreno Jan 15, 2025

connoramoreno Jan 16, 2025

Edgar-21 Jan 21, 2025

connoramoreno left a comment

connoramoreno Jan 22, 2025

Edgar-21 Jan 27, 2025

connoramoreno commented Jan 24, 2025 •

edited

Loading

gonuke commented Jan 24, 2025

connoramoreno left a comment

connoramoreno Jan 27, 2025

connoramoreno Jan 27, 2025

gonuke left a comment

Edgar-21 commented Jan 29, 2025

connoramoreno left a comment

connoramoreno left a comment

connoramoreno commented Jan 31, 2025

gonuke left a comment

		@@ -18,10 +18,6 @@ class MagnetSet(object):

		Arguments:
		coils_file (str): path to coil filament data file.

split MagnetSet and MagnetCoil #182

split MagnetSet and MagnetCoil #182

Conversation

Edgar-21 commented Dec 27, 2024 • edited Loading

connoramoreno left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connoramoreno left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connoramoreno commented Jan 24, 2025 • edited Loading

gonuke commented Jan 24, 2025

connoramoreno left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gonuke left a comment

Choose a reason for hiding this comment

Edgar-21 commented Jan 29, 2025

connoramoreno left a comment

Choose a reason for hiding this comment

connoramoreno left a comment

Choose a reason for hiding this comment

connoramoreno commented Jan 31, 2025

gonuke left a comment

Choose a reason for hiding this comment

Edgar-21 commented Dec 27, 2024 •

edited

Loading

connoramoreno commented Jan 24, 2025 •

edited

Loading