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

refactor: Reuse computeSinglePhaseFlux #3283

Merged
merged 8 commits into from
Aug 22, 2024
Merged

Conversation

paveltomin
Copy link
Contributor

No description provided.

@paveltomin paveltomin changed the title Reuse computeSinglePhaseFlux refactor: Reuse computeSinglePhaseFlux Aug 14, 2024
@paveltomin paveltomin self-assigned this Aug 14, 2024
@paveltomin paveltomin added ci: run CUDA builds Allows to triggers (costly) CUDA jobs ci: run integrated tests Allows to run the integrated tests in GEOS CI labels Aug 14, 2024
Copy link

codecov bot commented Aug 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.02%. Comparing base (540f606) to head (1060c7e).
Report is 88 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3283      +/-   ##
===========================================
+ Coverage    56.00%   56.02%   +0.01%     
===========================================
  Files         1053     1053              
  Lines        89168    89123      -45     
===========================================
- Hits         49935    49927       -8     
+ Misses       39233    39196      -37     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@paveltomin paveltomin added flag: ready for review flag: requires rebaseline Requires rebaseline branch in integratedTests and removed ci: run CUDA builds Allows to triggers (costly) CUDA jobs labels Aug 14, 2024
@paveltomin
Copy link
Contributor Author

somehow requires rebaseline due to some numerical diffs, for example:

Error: /Problem/domain/MeshBodies/mesh/meshLevels/Level0/ElementRegions/elementRegionsGroup/Domain/elementSubRegions/cb1/deltaPressure
	Arrays of types float64 and float64 have 21 values of which 10 fail both the relative and absolute tests.
		Max absolute difference is at index (np.int64(19),): value = 0.00641802791506052, base_value = 0.006416955031454563
		Max relative difference is at index (np.int64(20),): value = 0.00043659377843141556, base_value = 0.00043603498488664627
	Statistics of the q values greater than 1.0 defined by absolute tolerance: N = 4
		max = 10728.836059570312, mean = 867.0169682729812, std = 2522.583500320097
		max is at index (np.int64(19),), value = 0.00641802791506052, base_value = 0.006416955031454563
	Statistics of the q values greater than 1.0 defined by relative tolerance: N = 6
		max = 1585.5540718947793, mean = 88.88597318065965, std = 336.763511641095
		max is at index (np.int64(18),), value = 0.09559668321162462, base_value = 0.09559607692062855

Copy link
Collaborator

@CusiniM CusiniM left a comment

Choose a reason for hiding this comment

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

why was this even different?
we should add a unitTest for this computeFlux function.

@paveltomin paveltomin added ci: run CUDA builds Allows to triggers (costly) CUDA jobs type: cleanup / refactor Non-functional change (NFC) and removed flag: ready for review labels Aug 15, 2024
@ryar9534
Copy link
Contributor

why was this even different? we should add a unitTest for this computeFlux function.

We should probably note somewhere to add the unit test, as if this gets merged I have a feeling we will forget

@rrsettgast
Copy link
Member

@paveltomin @jhuang2601 @CusiniM
so does this get taken off the queue until a unit test is created?

@CusiniM
Copy link
Collaborator

CusiniM commented Aug 22, 2024

@paveltomin @jhuang2601 @CusiniM so does this get taken off the queue until a unit test is created?
okay let's just add an issue.

@paveltomin
Copy link
Contributor Author

@paveltomin @jhuang2601 @CusiniM so does this get taken off the queue until a unit test is created?

issue is here #3297

i will merge it for now

@rrsettgast rrsettgast merged commit ececf78 into develop Aug 22, 2024
22 checks passed
@rrsettgast rrsettgast deleted the pt/re-use-computeflux branch August 22, 2024 16:19
rrsettgast pushed a commit that referenced this pull request Sep 17, 2024
* Reuse computeSinglePhaseFlux

* Update SinglePhaseFVMKernels.hpp
rrsettgast pushed a commit that referenced this pull request Sep 17, 2024
* Reuse computeSinglePhaseFlux

* Update SinglePhaseFVMKernels.hpp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci: run CUDA builds Allows to triggers (costly) CUDA jobs ci: run integrated tests Allows to run the integrated tests in GEOS CI flag: requires rebaseline Requires rebaseline branch in integratedTests type: cleanup / refactor Non-functional change (NFC)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants