Add a function in material base class to check whether a property is active #26696

YaqiWang · 2024-01-30T20:56:57Z

Closes #26675.

YaqiWang · 2024-01-30T23:58:42Z

I think this is ready for review. @lindsayad and @NamjaeChoi

moosebuild · 2024-01-31T02:00:57Z

Job Documentation on 4246721 wanted to post the following:

View the site here

This comment will be updated on new commits.

moosebuild · 2024-01-31T02:52:15Z

Job Coverage on 4246721 wanted to post the following:

Framework coverage

	519d81	#26696 424672
	Total	Total	+/-	New
Rate	85.21%	85.21%	+0.00%	100.00%
Hits	99626	99628	+2	28
Misses	17292	17286	-6	0

Diff coverage report

Full coverage report

Modules coverage

Ray tracing

	519d81	#26696 424672
	Total	Total	+/-	New
Rate	95.29%	95.29%	-0.00%	100.00%
Hits	4327	4326	-1	1
Misses	214	214	-	0

Diff coverage report

Full coverage report

Full coverage reports

Reports

This comment will be updated on new commits.

GiudGiud

you set the status, but you dont use it. Didnt you mean to try to skip material property evaluations for inactive properties?

framework/include/materials/MaterialData.h

framework/src/problems/FEProblemBase.C

test/tests/materials/active_material_props/tests

framework/src/problems/FEProblemBase.C

YaqiWang · 2024-01-31T15:07:02Z

you set the status, but you dont use it. Didnt you mean to try to skip material property evaluations for inactive properties?

Yes. I only tested the function but I did not create a material use the function to skip evaluations. Actually I can make the derived test material I added in this PR do the skip.

YaqiWang · 2024-01-31T15:47:24Z

I also added more protection when calling isPropertyActive with mooseAssert. Because isPropertyActive is called in element loop and only developer will be interacting with this function, I did not make those protection as mooseError.

YaqiWang · 2024-01-31T15:52:36Z

@NamjaeChoi can you test this out with your PR in Griffin?

YaqiWang · 2024-01-31T16:08:04Z

Added some documentation.

framework/doc/content/syntax/Materials/index.md

GiudGiud

@lindsayad should we have a meeting on this?
Seems like we should figure out how to turn this extra work of keeping track which properties are active into some sort of optimization in moose not just in derived apps

YaqiWang · 2024-01-31T23:07:59Z

@lindsayad should we have a meeting on this? Seems like we should figure out how to turn this extra work of keeping track which properties are active into some sort of optimization in moose not just in derived apps

I think this has to be in computeQpProperties of individual materials. Moose has had the capability of whether call computeQpProperties of materials, right?

YaqiWang · 2024-01-31T23:13:33Z

Wait a moment... looks like MOOSE will call computeQpProperties even all properties of a material are inactive. This can be seen by adding another dummy material in the test input.

lindsayad · 2024-02-02T01:18:39Z

@YaqiWang see what you think of 96a61d2. It's an optimization changing the broader active properties to an unordered_set

YaqiWang · 2024-02-02T01:31:00Z

Looks good! Thanks Alex.

YaqiWang · 2024-02-02T02:42:22Z

The last commit will need patches for apps due to the interface change. I think having efficient MaterialBase::isPropertyActive is good enough.

lindsayad · 2024-02-02T05:05:41Z

Nothing a little c++ can't handle

lindsayad · 2024-02-02T05:19:33Z

The more I think about this the more I think we should just query the problem for whether a material property ID is active as opposed to doing these mat->setActiveProperties calls. If std::unordered_set::count is constant complexity on average, then it seems a little silly to me to essentially duplicate the active material property storage in FEProblemBase and in the material classes. If somehow a profile showed heavy cpu in std::unordered_set::count then that would motivate me more to consider local storage on the Material

YaqiWang · 2024-02-02T16:05:30Z

The more I think about this the more I think we should just query the problem for whether a material property ID is active as opposed to doing these mat->setActiveProperties calls. If std::unordered_set::count is constant complexity on average, then it seems a little silly to me to essentially duplicate the active material property storage in FEProblemBase and in the material classes. If somehow a profile showed heavy cpu in std::unordered_set::count then that would motivate me more to consider local storage on the Material

There is one thing in Material::subdomainChange might change your mind.

lindsayad · 2024-02-02T19:34:46Z

There is one thing in Material::subdomainChange might change your mind.

Ok you can see my follow-on commits in this PR that consolidates the active material property storage onto the materials. To keep CI clean this should be a two-stage process. Stage 1 will be to get #26720 merged. I then have a Griffin patch ready to go that will switch to the new prepareMaterials API as well as change the MatSetType to an unordered_set instead of the decltype involving getActiveMaterialProperties. Then once those two patches are merged, then this PR should test cleanly

YaqiWang · 2024-02-05T23:24:11Z

@lindsayad your change in Griffin has been merged into Griffin master.

lindsayad · 2024-02-06T00:05:03Z

👍 I'll fixup this PR momentarily

…currently active idaholab#26675

This allows for O(1) insertion of individual elements as opposed to O(log(N)). Similarly when marking active material properties in a given material this allows us to do a O(N) loop over the likely <= size of the supplied properties and do a O(1) search in the `needed_mat_props`

lindsayad · 2024-02-06T04:58:14Z

This should be ready for further review. Forward compatible relap-7 patch is here. Hopefully will get merged early tomorrow and then we can invalidate the failing targets here

GiudGiud

looks good.

it would be good to have some numbers to justify the optimization at some point @YaqiWang but no rush, this can come just for reporting in the TA report. thanks!

framework/include/materials/MaterialBase.h

framework/src/problems/FEProblemBase.C

Co-authored-by: Guillaume Giudicelli <guillaume.giudicelli@gmail.com>

lindsayad · 2024-02-07T00:40:44Z

thanks for the review!

YaqiWang · 2024-02-07T00:54:32Z

Really appreciate the helps!

YaqiWang requested review from lindsayad, roystgnr and loganharbour as code owners January 30, 2024 20:56

YaqiWang force-pushed the mat_prop_active_26675 branch 2 times, most recently from c6878db to cc46c1d Compare January 30, 2024 23:57

moosebuild added the PR: Failed but allowed label Jan 31, 2024

GiudGiud requested changes Jan 31, 2024

View reviewed changes

GiudGiud self-assigned this Jan 31, 2024

YaqiWang force-pushed the mat_prop_active_26675 branch from cc46c1d to 43ca4a9 Compare January 31, 2024 15:19

moosebuild removed the PR: Failed but allowed label Jan 31, 2024

YaqiWang force-pushed the mat_prop_active_26675 branch 2 times, most recently from c750091 to cdd6371 Compare January 31, 2024 15:45

YaqiWang force-pushed the mat_prop_active_26675 branch 2 times, most recently from 52d97e7 to 212304d Compare January 31, 2024 16:07

YaqiWang force-pushed the mat_prop_active_26675 branch from 212304d to 87b3f03 Compare January 31, 2024 17:52

moosebuild added the PR: Failed but allowed label Jan 31, 2024

GiudGiud reviewed Jan 31, 2024

View reviewed changes

framework/doc/content/syntax/Materials/index.md Outdated Show resolved Hide resolved

GiudGiud reviewed Jan 31, 2024

View reviewed changes

YaqiWang force-pushed the mat_prop_active_26675 branch from 87b3f03 to 80a7c1d Compare January 31, 2024 23:00

moosebuild removed the PR: Failed but allowed label Jan 31, 2024

moosebuild added the PR: Failed but allowed label Feb 1, 2024

YaqiWang requested a review from joshuahansel as a code owner February 2, 2024 01:17

lindsayad force-pushed the mat_prop_active_26675 branch 2 times, most recently from 63f34b2 to e6c98ae Compare February 2, 2024 05:30

lindsayad force-pushed the mat_prop_active_26675 branch from e6c98ae to c19d4e4 Compare February 2, 2024 18:00

lindsayad mentioned this pull request Feb 2, 2024

Stage 1 of active material property refactor #26720

Merged

lindsayad force-pushed the mat_prop_active_26675 branch from 97bf2ee to 5ba3795 Compare February 2, 2024 22:27

YaqiWang and others added 5 commits February 5, 2024 16:50

interface materials are managed in a separate warehouse idaholab#26675

70ffb27

add a function in material base class to check whether a property is …

6f1cc29

…currently active idaholab#26675

Refactor active material property APIs

ba0bfd3

Do not use thread-unsafe std::vector<bool>

ce764c9

lindsayad force-pushed the mat_prop_active_26675 branch from 5ba3795 to ce764c9 Compare February 6, 2024 00:57

lindsayad requested a review from GiudGiud February 6, 2024 04:57

GiudGiud reviewed Feb 6, 2024

View reviewed changes

framework/include/materials/MaterialBase.h Outdated Show resolved Hide resolved

framework/include/materials/MaterialBase.h Show resolved Hide resolved

framework/src/problems/FEProblemBase.C Outdated Show resolved Hide resolved

Improve doxygen/comment wording

4246721

Co-authored-by: Guillaume Giudicelli <guillaume.giudicelli@gmail.com>

GiudGiud approved these changes Feb 6, 2024

View reviewed changes

moosebuild added the PR: Failed but allowed label Feb 7, 2024

lindsayad merged commit bb2a44c into idaholab:next Feb 7, 2024
47 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a function in material base class to check whether a property is active #26696

Add a function in material base class to check whether a property is active #26696

YaqiWang commented Jan 30, 2024 •

edited

Loading

YaqiWang commented Jan 30, 2024

moosebuild commented Jan 31, 2024 •

edited

Loading

moosebuild commented Jan 31, 2024 •

edited

Loading

GiudGiud left a comment

YaqiWang commented Jan 31, 2024 •

edited

Loading

YaqiWang commented Jan 31, 2024

YaqiWang commented Jan 31, 2024

YaqiWang commented Jan 31, 2024

GiudGiud left a comment

YaqiWang commented Jan 31, 2024

YaqiWang commented Jan 31, 2024

lindsayad commented Feb 2, 2024

YaqiWang commented Feb 2, 2024

YaqiWang commented Feb 2, 2024 •

edited

Loading

lindsayad commented Feb 2, 2024

lindsayad commented Feb 2, 2024

YaqiWang commented Feb 2, 2024

lindsayad commented Feb 2, 2024

YaqiWang commented Feb 5, 2024

lindsayad commented Feb 6, 2024

lindsayad commented Feb 6, 2024

GiudGiud left a comment

lindsayad commented Feb 7, 2024

YaqiWang commented Feb 7, 2024

Add a function in material base class to check whether a property is active #26696

Add a function in material base class to check whether a property is active #26696

Conversation

YaqiWang commented Jan 30, 2024 • edited Loading

YaqiWang commented Jan 30, 2024

moosebuild commented Jan 31, 2024 • edited Loading

moosebuild commented Jan 31, 2024 • edited Loading

Framework coverage

Modules coverage

Ray tracing

Full coverage reports

GiudGiud left a comment

Choose a reason for hiding this comment

YaqiWang commented Jan 31, 2024 • edited Loading

YaqiWang commented Jan 31, 2024

YaqiWang commented Jan 31, 2024

YaqiWang commented Jan 31, 2024

GiudGiud left a comment

Choose a reason for hiding this comment

YaqiWang commented Jan 31, 2024

YaqiWang commented Jan 31, 2024

lindsayad commented Feb 2, 2024

YaqiWang commented Feb 2, 2024

YaqiWang commented Feb 2, 2024 • edited Loading

lindsayad commented Feb 2, 2024

lindsayad commented Feb 2, 2024

YaqiWang commented Feb 2, 2024

lindsayad commented Feb 2, 2024

YaqiWang commented Feb 5, 2024

lindsayad commented Feb 6, 2024

lindsayad commented Feb 6, 2024

GiudGiud left a comment

Choose a reason for hiding this comment

lindsayad commented Feb 7, 2024

YaqiWang commented Feb 7, 2024

YaqiWang commented Jan 30, 2024 •

edited

Loading

moosebuild commented Jan 31, 2024 •

edited

Loading

moosebuild commented Jan 31, 2024 •

edited

Loading

YaqiWang commented Jan 31, 2024 •

edited

Loading

YaqiWang commented Feb 2, 2024 •

edited

Loading