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

Add a virtual PhaseFunctionBase class #6105

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lhy11009
Copy link
Contributor

Pull Request Checklist. Please read and check each box with an X. Delete any part not applicable. Ask on the forum if you need help with any step.

Introduction to the PR

In this pull request, I introduce a new class, PhaseFunctionBase, which serves as a pure virtual base class for different implementations of the PhaseFunction class. My motivation for this design stems from the need to create a PhaseFunction that returns discrete values (e.g., 0 or 1) based on a table lookup.

To provide more context, I aim to reuse much of the existing structure from the PhaseFunction class originally implemented by @gassmoeller . For instance, the vector structure for storing phase function values will remain intact, and we will continue parsing the relevant data from the parameter file.

The main changes involve overriding several key functions, specifically:

  • compute_value: This will now perform a table lookup instead of using continuous functions.
  • compute_derivative: Since an analytical solution is not applicable in this case, we may adopt a perturbation-based approach, similar to how viscosity derivatives are computed in the material model.
  • declare_parameters and parse_parameters: These will be reloaded to accommodate the changes in functionality.

As you can see in the current format of the PR, the new class is purely virtual, which provides flexibility for future phase function implementations. However, I am unsure if making the base class purely virtual is the best approach for this problem. I would appreciate feedback on whether this is the right path forward.

Before your first pull request:

For all pull requests:

For new features/models or changes of existing features:

  • I have tested my new feature locally to ensure it is correct.
  • [] I have added a changelog entry in the doc/modules/changes directory that will inform other users of my change.

@lhy11009
Copy link
Contributor Author

lhy11009 commented Oct 22, 2024

Another thread: Question on PhaseFunction<dim>::compute_derivative Implementation

I have a question regarding the existing implementation of PhaseFunction<dim>::compute_derivative. While reviewing the code, I noticed that if the transition width is set to 0.0, the function returns 0.0. Conceptually, I would expect that a transition width of 0.0 represents a very sharp transition, so I'm wondering why the value is set to 0.0 in this case.

Could you provide some clarification on the reasoning behind this behavior?

@gassmoeller
Copy link
Member

However, I am unsure if making the base class purely virtual is the best approach for this problem.

I think whether to make a base class depends on how the phase functions are going to be used. The main advantage of having a base class is that you can use a phase function without knowing its type. I think the main question will be what functions are mandatory for a phase function (those should be in the base class) and which ones depends on the actual implementation (these should only be in the derived classes). For example the transition_slope function will be hard/impossible to implement for a phase function that is not based on analytic functions for phase functions. I would suggest you start implementing your new class based on the table and observe which functions should be shared with the existing phase function (those functions should go in the base class) and which functions are specific to each phase function implementation (those should only be in the derived classes).

Could you provide some clarification on the reasoning behind this behavior?

I do not know if I remember (or whether that was a conscious decision). For a width of 0 we can obviously not meaningfully compute a derivative, so it could also be fair to crash if we are asked for the derivative. But I do not know how this is used so you would have to take a look what that change would mean for the users of this class.

@lhy11009
Copy link
Contributor Author

I would suggest you start implementing your new class based on the table and observe which functions should be shared with the existing phase function.
Sure, I'll move forward in that direction instead.

I do not know if I remember (or whether that was a conscious decision). For a width of 0, we can obviously not meaningfully compute a derivative,
Yes, I see. Let me return to this one later, for the lookup table, it would also be hard to compute a derivative.

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.

2 participants