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

Update integration with CAD-to-DAGMC v0.6.2 for oo_version #80

Closed
wants to merge 1 commit into from

Conversation

connoramoreno
Copy link
Collaborator

@connoramoreno connoramoreno commented Apr 9, 2024

Updates syntax in export_cad_to_dagmc method of InVesselBuild class for the oo_version branch to reflect update to CAD-to-DAGMC dependency. The test script for InVesselBuild is also updated to incorporate testing of the in-vessel build DAGMC export.

It should be noted that while the new syntax incorporated is correct, at the initial commit for this PR, there are issues when faceting a full period of the test/example geometry.

Closes #78.

@connoramoreno connoramoreno linked an issue Apr 9, 2024 that may be closed by this pull request
@connoramoreno connoramoreno self-assigned this Apr 9, 2024
@connoramoreno connoramoreno linked an issue Apr 9, 2024 that may be closed by this pull request
@connoramoreno
Copy link
Collaborator Author

With the changes to unit tests introduced in PR #81, the issue documented in #79 no longer occurs. Are we comfortable considering the issue resolved, at least as far as this PR is concerned?

@gonuke
Copy link
Member

gonuke commented Apr 10, 2024

I think that's probably OK, but maybe we don't close #79 yet...

@connoramoreno
Copy link
Collaborator Author

That makes sense to me!

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 question/concern here.... not sure if it exists in the PR we already merged for the old version. It seems like the decision of cad_to_dagmc bind this data together more loosely is dangerous and risks material tags being out of sync with the objects. (@shimwell ?)

Comment on lines +355 to +360
for component in self.Components.values():
model.add_cadquery_object(component)

material_tags = [
component['mat_tag'] for component in self.radial_build.values()
]
Copy link
Member

Choose a reason for hiding this comment

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

Do the relative orders of these things matter? One loops over self.Components and one over self.radial_build. Presumably those things are both built in the same order, but nothing guarantees it...?

@shimwell
Copy link
Member

One question/concern here.... not sure if it exists in the PR we already merged for the old version. It seems like the decision of cad_to_dagmc bind this data together more loosely is dangerous and risks material tags being out of sync with the objects. (@shimwell ?)

It is a bit riskier yes, it does assume the user knows the number of solids in each added cadquery assembly or step file. The number of material tags is checked for the length of the total volumes in the solids but not at each stage of addition.

I'm thinking I should go back to old api, please hold of on merging this.

@gonuke
Copy link
Member

gonuke commented Apr 10, 2024

It's not just about the number, it's about the order! This API relies on the user to keep track of this data in a way that preserves this binding.

@shimwell
Copy link
Member

shimwell commented Apr 15, 2024

I've changed cad-to-dagmc back to requesting material_tags when adding stp files of cadquery objects. I think this PR is no longer needed. Sorry for the mess

@shimwell shimwell closed this Apr 15, 2024
@connoramoreno connoramoreno deleted the update-cad-to-dagmc branch April 26, 2024 20:13
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.

Integration with Updated CAD-to-DAGMC
3 participants