-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Split magnet set object #173
base: main
Are you sure you want to change the base?
Conversation
tests are passing locally :( I did make some changes to the tests to accommodate the changes in this PR, maybe the CI is using the old versions of the tests? I'll look more into it soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is still in draft stage, but here are some general comments and suggestions
@@ -192,3 +209,22 @@ def export_dagmc_cubit_native( | |||
# exports | |||
if delete_upon_export: | |||
cubit.cmd(f"delete mesh volume all propagate") | |||
|
|||
|
|||
def cubit_importer(filename, import_dir=""): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be best to modify this function name to start with a verb to follow our established convention/best practices, such as import_cubit
or something similar.
@@ -36,7 +36,7 @@ def import_step_cubit(filename, import_dir): | |||
"""Imports STEP file into Coreform Cubit. | |||
|
|||
Arguments: | |||
filename (str): name of STEP input file, excluding '.step' extension. | |||
filename (str): name of STEP input file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason we've removed the specification that the '.step' extension should be excluded? The function still needs that to be the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Path(filename).with_suffix(".step")
replaces whatever suffix is there with .step or adds it if no suffix is present
>>> from pathlib import Path
>>> test = Path('test.py')
>>> test
PosixPath('test.py')
>>> test.with_suffix('.step')
PosixPath('test.step')
>>>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I didn't know that about with_suffix
. Neat
@@ -55,7 +55,7 @@ def export_step_cubit(filename, export_dir=""): | |||
"""Export CAD solid as a STEP file via Coreform Cubit. | |||
|
|||
Arguments: | |||
filename (str): name of STEP output file, excluding '.step' extension. | |||
filename (str): name of STEP output file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above comment.
def import_cub5_cubit(filename, import_dir): | ||
"""Imports cub5 file with Coreform Cubit with default import settings. | ||
Arguments: | ||
filename (str): name of cub5 input file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be best to specify that the '.cub5' extension should be excluded from the filename
def export_cub5(filename, export_dir=""): | ||
"""Export cub5 representation of model (native Cubit format). | ||
|
||
Arguments: | ||
filename (str): name of cub5 output file, excluding '.cub5' extension. | ||
filename (str): name of cub5 output file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above comments for STEP file extensions
if cubit_io.initialized: | ||
cubit.cmd("new") | ||
else: | ||
cubit_io.init_cubit() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? Not sure how/if pytest isolates these tests
if cubit_io.initialized: | ||
cubit.cmd("new") | ||
else: | ||
cubit_io.init_cubit() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question as above
if cubit_io.initialized: | ||
cubit.cmd("new") | ||
else: | ||
cubit_io.init_cubit() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above
@@ -83,3 +99,38 @@ def test_magnet_exports(coil_set): | |||
assert Path("magnet_mesh.h5m").exists() | |||
|
|||
remove_files() | |||
|
|||
|
|||
def test_magnets_from_geom_cubit_import(coil_set_from_geom): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Call remove_files
before and after function calls. In this case, step_import.log
and maybe some other log file for the CUB5 import will be created.
assert coil_set_from_geom.volume_ids == volume_ids_exp | ||
|
||
|
||
def test_magnets_from_geom_exports(coil_set_from_geom): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Call remove_files
before and after function calls
First crack at the class hierarchy disussed a couple weeks back - not sure the best way to handle things that you might want to be part of the init method for both classes? I just left those variables (like logger) in the init for each class. This felt a bit like defining things in two places.