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

move viscosity averaging function #6008

Merged
merged 2 commits into from
Aug 21, 2024

Conversation

bobmyhill
Copy link
Member

@bobmyhill bobmyhill commented Aug 13, 2024

This PR moves the get_averaging_operation_for_viscosity() function from material_model/rheology/elasticity.cc into material_model/interface.cc.

This is a more logical place for the function, and makes it more easily accessible by rheology and material_model modules.

Addresses a single comment here: #5984 (comment)

@bobmyhill
Copy link
Member Author

/rebuild

@bobmyhill
Copy link
Member Author

/rebuild

Copy link
Member

@gassmoeller gassmoeller left a comment

Choose a reason for hiding this comment

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

would aspect/material_model/utilities.h be a better place? you only want to make this accessible to other material models than just elasticity, correct?

@bobmyhill
Copy link
Member Author

@gassmoeller that is probably true, but it would require more code refactoring. interface.h contains the definition of the AveragingOperation enum, and also includes <aspect/material_model/utilities.h>. So to avoid circular dependencies I would have to move the enum definition and possibly some of the other functions over to utilities.

I'm happy to either stick with what I have here or do the refactoring.

Copy link
Member

@gassmoeller gassmoeller left a comment

Choose a reason for hiding this comment

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

Ah, I see. I am ok with keeping it in the interface for now then. However, please extend the documentation of the function similar to what I suggest.


/**
* Parse an AveragingOperation and alias to an AveragingOperation
* that is appropriate for viscosity averaging.
Copy link
Member

Choose a reason for hiding this comment

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

Please extend the description of the function, I had to read the implementation to understand what is happening here. Something like the following:

       * Parse an AveragingOperation and alias to an AveragingOperation
       * that only averages viscosity. In the case of any averaging operation 
       * that averages all properties the corresponding operation for only
       * viscosity is selected (e.g. 'harmonic_average_only_viscosity' instead
       * of 'harmonic_average'). This is useful in places where averaging is
       * performed manually on only the viscosity property.

Copy link
Member

@gassmoeller gassmoeller left a comment

Choose a reason for hiding this comment

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

👍 good to go if the testers are happy.

@bobmyhill bobmyhill merged commit a3f187a into geodynamics:main Aug 21, 2024
7 of 8 checks passed
@bobmyhill bobmyhill deleted the move_eta_averaging branch August 21, 2024 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants