Skip to content
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

Skip imprinting #32

Merged
merged 11 commits into from
Feb 21, 2024
Merged

Skip imprinting #32

merged 11 commits into from
Feb 21, 2024

Conversation

Edgar-21
Copy link
Contributor

This PR skips the imprint step and instead merges the coincident surfaces using the fact that parastell will import the volumes in inside to outside order. (and some cubit geometry queries)

Likely we will want this to be some sort of setting, right now it just completely replaces the imprint/merge commands, maybe another item in the export dictionary would be the best place to put the option to toggle this, open to suggestions.

For reference imprinting a build with 10 layers took about 50 minutes to imprint for me yesterday, this takes about 5 seconds to merge all coincident surfaces in the example script (7 surfaces to merge)

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure we want to permanently change this, but make it an option

parastell.py Outdated Show resolved Hide resolved
Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more coding suggetions.

parastell.py Outdated Show resolved Hide resolved
parastell.py Outdated Show resolved Hide resolved
@Edgar-21 Edgar-21 mentioned this pull request Feb 15, 2024
Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more suggestions - nearly there...

magnet_coils.py Show resolved Hide resolved
parastell.py Outdated Show resolved Hide resolved
parastell.py Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One little request to tweak a comment and we should be ready to go

parastell.py Outdated Show resolved Hide resolved
@gonuke
Copy link
Member

gonuke commented Feb 20, 2024

I'm happy to approve this, but wonder if someone who can actively test it should be responsible for merging. (@connoramoreno @FusionSandwich ?)

@connoramoreno
Copy link
Collaborator

I tested and confirmed that the correct surfaces are being merged. Slick way of doing things @Edgar-21, nicely done. Only thing I'd say before merging has to do with code formatting. Typically, we like to use some kind of standardized style (usually PEP8 in this research group) for things like docstrings, comments, text wrap, line skips, etc. Very minor though, not sure what @gonuke's thoughts are on this topic.

@gonuke
Copy link
Member

gonuke commented Feb 20, 2024

Definitely good to get used to PEP8 formatting ASAP

@Edgar-21
Copy link
Contributor Author

Made some changes to comments, variable names, line lengths and newlines based on what I read in PEP8, please let me know if I missed anything

@gonuke
Copy link
Member

gonuke commented Feb 21, 2024

Made some changes to comments, variable names, line lengths and newlines based on what I read in PEP8, please let me know if I missed anything

You should look into a PEP8 formatter for your editor that will do this automatically...

@gonuke gonuke merged commit ef52b67 into svalinn:main Feb 21, 2024
@Edgar-21 Edgar-21 deleted the skip_imprinting branch April 19, 2024 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants