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 time transformation #29640

Open
wants to merge 2 commits into
base: next
Choose a base branch
from
Open

Conversation

dschwen
Copy link
Member

@dschwen dschwen commented Jan 3, 2025

Closes #29639

@dschwen dschwen force-pushed the solution_uo_29639 branch from 5e1aea7 to a097131 Compare January 3, 2025 22:02
@idaholab idaholab deleted a comment from moosebuild Jan 3, 2025
@dschwen dschwen marked this pull request as ready for review January 3, 2025 22:03
@dschwen dschwen requested a review from lindsayad as a code owner January 3, 2025 22:03
@moosebuild
Copy link
Contributor

Job Precheck, step Clang format on a097131 wanted to post the following:

Your code requires style changes.

A patch was auto generated and copied here
You can directly apply the patch by running, in the top level of your repository:

curl -s https://mooseframework.inl.gov/docs/PRs/29640/clang_format/style.patch | git apply -v

Alternatively, with your repository up to date and in the top level of your repository:

git clang-format 7d4ab78f22eae799c7d28a9f675a33e886d7831e

@dschwen dschwen force-pushed the solution_uo_29639 branch 2 times, most recently from 36bc35d to 061e9c5 Compare January 3, 2025 22:59
@moosebuild
Copy link
Contributor

Job OpenMPI on 061e9c5 : invalidated by @dschwen

Unrelated failure?

@moosebuild
Copy link
Contributor

moosebuild commented Jan 4, 2025

Job Documentation, step Docs: sync website on 1cc4a29 wanted to post the following:

View the site here

This comment will be updated on new commits.

@moosebuild
Copy link
Contributor

moosebuild commented Jan 4, 2025

Job Coverage, step Generate coverage on 1cc4a29 wanted to post the following:

Framework coverage

1e1768 #29640 1cc4a2
Total Total +/- New
Rate 85.25% 85.25% -0.00% 66.67%
Hits 108011 108015 +4 18
Misses 18681 18682 +1 9

Diff coverage report

Full coverage report

Modules coverage

Coverage did not change

Full coverage reports

Reports

Warnings

  • framework new line coverage rate 66.67% is less than the suggested 90.0%

This comment will be updated on new commits.

GiudGiud
GiudGiud previously approved these changes Jan 4, 2025
framework/src/userobjects/SolutionUserObject.C Outdated Show resolved Hide resolved
@GiudGiud GiudGiud dismissed their stale review January 4, 2025 10:00

Phase field test shows there's a solution initialization issue. Should add a call to initial setup on the new transformation function

@GiudGiud
Copy link
Contributor

GiudGiud commented Jan 4, 2025

function* not solution

@dschwen
Copy link
Member Author

dschwen commented Jan 4, 2025

function* not solution

No the time in the solution in the Solution user object

@GiudGiud
Copy link
Contributor

GiudGiud commented Jan 4, 2025

the function that gives you the transformation, probably needs a initialSetup call on it before it gets used in that phase field test

@dschwen
Copy link
Member Author

dschwen commented Jan 4, 2025

I'll clarify the doc string. I'm surprised at the initialization issue. No initialization was performed before the pr, and the change should not affect behavior with the default function (identity). :-/

@GiudGiud
Copy link
Contributor

GiudGiud commented Jan 4, 2025

before the PR, you did not create a parsed function like this in a parameter? I m not sure there were any parsed functions involved in that object, which executes rather early

@dschwen
Copy link
Member Author

dschwen commented Jan 4, 2025

Fingers crossed that the early function initialization doesn't break other stuff...

@dschwen dschwen force-pushed the solution_uo_29639 branch from 97cd84a to ebce148 Compare January 5, 2025 15:46
@dschwen dschwen requested a review from zachmprince as a code owner January 5, 2025 15:46
@dschwen
Copy link
Member Author

dschwen commented Jan 5, 2025

If this doesn't pass I'm rolling back the changes I made to SolUO (moving initialization from initialSetup to the constructor). The root problem is the function requiring access to the userobject to get the name of the variable in the solution file. This currently prevents moving Function initialSetup before UO initialSetup. IMO we should just get rid of that small convenience where the user doesn't have to specify a variable name anywhere, as long as the solution only contains a single variable. That scenario seems like an edge case, and having the variable stated explicitly in the input seems more expressive.

@dschwen dschwen force-pushed the solution_uo_29639 branch 2 times, most recently from 6d489af to 1cc4a29 Compare January 6, 2025 05:10
@dschwen dschwen force-pushed the solution_uo_29639 branch from 1cc4a29 to 0d9b1a1 Compare January 6, 2025 23:07
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.

Sample SolutionUserObject at a specific arbitrary _time_
3 participants