-
Notifications
You must be signed in to change notification settings - Fork 0
feat trial protocol for writing #414
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #414 +/- ##
==========================================
+ Coverage 87.67% 89.96% +2.29%
==========================================
Files 55 61 +6
Lines 2052 1904 -148
==========================================
- Hits 1799 1713 -86
+ Misses 253 191 -62 🚀 New features to boost your workflow:
|
…s/pymaterials-manager into rmanno/trial_protocol_write
|
@koubaa I have starting to sketch a possible way for writing to ls-dyna keyword. Could you please have a look at it? Do you have a better idea on how to move forward? (I have also added in herein the changes of your pr) |
|
I think the matml writer and the solver writers are inherently the same kind of operation (an algorithm that visits a material and emits the data into some structure). I like that you've decoupled the material models from the mapdl writer but I think it would be nice to use the visitor pattern like so: So that any visitor can be applied on a material model or list of material models. |
Thanks Mohamed, I appreciate you taking a look at it. |
This is completely fine. The visitor can visit three material models and produce one, two, three, or twelve keywords. It doesn't have to be one keyword per model in the visitor pattern, since the visitor class will have state that is maintained across visits. It is an implementation detail of the visitor for how to accomplish this.
Sounds like you are talking here about reading a matml file, which should not use a visitor pattern.
I hesitate to decide at the start, since designs often have to change multiple times to accomodate new information. My preference is to keep the dyna conversion in one place so that changes to its structure do not need to affect other parts of the library. |
okay I think I start seeing the wider picture. I undestand now the added value representing things in this way. What do you think if I merge this where we have an initial separation between material model and solver representation and I add an issue with the discussion we are having here? So I can tackle it in a new pr. This way I can start having a small refactor MD to remove the write to dyna of simple materials. |
|
Fine with me! |
Andy-Grigg
left a comment
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.
Some nitpicky comments, mainly around naming and package structure. Just trying to think about which aspects of the implementation are relevant at each level of abstraction.
Overall this is good, and I really like the fact that we're now separating the abstract model definition from the writing logic. As you say, there will be more iteration on some of the subtleties, but having this initial separation of logic should make that iteration easier.
| @@ -0,0 +1,58 @@ | |||
| # Copyright (C) 2022 - 2025 ANSYS, Inc. and/or its affiliates. | |||
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.
Some nitpicky suggestions about the layout of these writer classes:
- This module could just be called
writer, the fact that it's in thefluentmodule tells you it's for fluent anyway. It could then be imported asfrom .util.fluent import writer as FluentWriter. Same for the other writer implementations. - Would it make more sense to rename the
utilfolder towriters, or at least to move the writer code into a newwriterssubpackage.
| @@ -0,0 +1,109 @@ | |||
| # Copyright (C) 2022 - 2025 ANSYS, Inc. and/or its affiliates. | |||
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.
Another nitpick, I think this is a private module? i.e. it's only ever imported by writer_ls_dyna.py. In that case, the modules in this folder could be renamed to:
- writer.py
- _utils.py
This would make the import statements more concise, and would help developers understand that there's nothing in this module that they should care about unless they're dealing with the dyna writer internals.
| """FieldInfo for dependent parameters in material models.""" | ||
|
|
||
| def __init__(self, *, matml_name=None, **kwargs): | ||
| def __init__( |
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.
It's a shame that we have references to specific solvers here. An alternative would be for a solver-agnostic name here, and then for each writer implementation to define some way of getting from the solver-agnostic name to the name that solver expects. This could be a mapping or some kind of algorithm, but would be on the writer code to implement.
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 agree with you Andy, In the new version we will try to get rid of it. Still I think each material model attribute need to specify if it is supported by a specific solver. This for the user to know what to provide and what not for a specific model and specific solver.
In this PR the write material method is refactored as sketched in the following code.
A first draft of the LS-Dyna writer is sketched as well.