[Refactor] Particle variables and functions handling #654

kks32 · 2020-06-29T18:06:09Z

Describe the PR
This is an alternative implementation of particle refactor.

After some more consideration, I am wondering about the possibility to derive particle types, but only keep the common particle functions within the class, and merge all independent/specialized functions to be outside the class. This will help to initialize the classes with the right types, and also in MPI transfer, as the type is known. We derived data of individual Particle types but keep specialized particle functions separately. Maybe the particle functions may have to stay in the same namespace.

Related Issues/PRs
#637 and #650

Additional context
Specialized functions will be still separate.

Following are the run-times for 3D hydrostatic column 10k steps on Stampede2

Branch	TACC runtime (s)
develop	742.6
prop map + free fn	1280
vector + free fn map	865
flatmap + free fn	972

codecov · 2020-06-29T18:23:07Z

Codecov Report

Merging #654 into develop will increase coverage by 0.00%.
The diff coverage is 97.45%.

@@            Coverage Diff            @@
##           develop     #654    +/-   ##
=========================================
  Coverage    96.66%   96.66%            
=========================================
  Files          123      124     +1     
  Lines        25375    25641   +266     
=========================================
+ Hits         24527    24785   +258     
- Misses         848      856     +8

Impacted Files	Coverage Δ
include/node_base.h	`100.00% <ø> (ø)`
include/particles/particle_base.h	`100.00% <ø> (ø)`
include/solvers/mpm_base.tcc	`73.21% <50.00%> (-0.48%)`	⬇️
include/particles/particle.tcc	`93.24% <95.56%> (-0.89%)`	⬇️
include/node.tcc	`95.12% <96.18%> (-0.86%)`	⬇️
include/mesh.tcc	`83.94% <100.00%> (+0.04%)`	⬆️
include/node.h	`100.00% <100.00%> (ø)`
include/particles/particle.h	`100.00% <100.00%> (ø)`
include/particles/particle_base.tcc	`100.00% <100.00%> (ø)`
include/particles/particle_functions.tcc	`100.00% <100.00%> (ø)`
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b20da5...a0bbd15. Read the comment docs.

…/particles

kks32 · 2020-07-22T12:41:04Z

After considering the speed-ups and different approaches, the use of scalar and vector properties still is 30% slower than using private variables. The advantage of scalar/vector properties is we'll have fewer functions and less repetition of code. On the other hand, defining private variables and deriving particle classes has more function definitions and repeating code, but is faster. I'd like to propose a vote, use emojis to vote between different options, please use comments below if you like to elaborate on your thoughts. @bodhinandach @tianchiTJ @thiagordonho @cw646 @ezrayst @jgiven100 @WeijianLiang

Use derived particle types with private variables (:rocket:)
Solid particles will remain as is, while we add scalar and vector properties to derived particle types (:smile:) PR [Refactor] Adding particles properties #673
Cleaner but slower design with free functions and scalar/vector properties for all particle types (:heart:) PR [Refactor] Particle variables and functions handling #654
Go back to the drawing board (:eyes:)

WeijianLiang · 2020-07-22T15:27:07Z

Thanks Krishna, I would prefer the use of scalar and vector properties which has a more clear structure.

kks32 · 2020-07-22T15:50:34Z

@WeijianLiang is that option 2 or 3? Could you please vote with a reaction emoji 😄 for option 2 and ❤️ for option 3. Thanks!

yliang-sn · 2020-07-22T19:24:07Z

After considering the speed-ups and different approaches, the use of scalar and vector properties still is 30% slower than using private variables. The advantage of scalar/vector properties is we'll have fewer functions and less repetition of code. On the other hand, defining private variables and deriving particle classes has more function definitions and repeating code, but is faster. I'd like to propose a vote, use emojis to vote between different options, please use comments below if you like to elaborate on your thoughts. @bodhinandach @tianchiTJ @thiagordonho @cw646 @ezrayst @jgiven100 @WeijianLiang

Use derived particle types with private variables (🚀)

Solid particles will remain as is, while we add scalar and vector properties to derived particle types (😄)

Cleaner but slower design with free functions and scalar/vector properties for all particle types (❤️)

Go back to the drawing board (👀)

For 2, are we just dealing with the scalar and vector properties to the derived particle types? If we have other types of quantities, do we still use the private variables?

kks32 · 2020-07-22T19:35:25Z

@yliang-sn everything will be held within scalar/vector/tensor/boolean

yliang-sn · 2020-07-22T19:44:50Z

@yliang-sn everything will be held within scalar/vector/tensor/boolean

Can I vote, too? If the private variables have the best efficiency, I prefer to 1. The structure is also clear enough to be read and extended. However, 2 is also fine for me.

kks32 · 2020-07-22T20:57:55Z

@yliang-sn yes, I couldn't find your handle, please vote.

…-geo/mpm into refactor/particles

kks32 · 2020-07-23T03:49:10Z

@thiagordonho @tianchiTJ @cw646 @WeijianLiang could you please use emoji reactions to vote on this comment please: #654 (comment)

ezrayst · 2020-07-23T06:43:51Z

Both @kks32 and @bodhinandach have been working hard on this issue so appreciate your work. I think it has been about 2 months now.

However, I feel that we need a unanimous decision on this refactor, or at least a 2/3 decision. The reason being is I don't want us to be in this position within 6 months where we are thinking to refactor again. I think the timeline on this issue has shown that this is a lot of work and everyone's progress is halted. Personally, if we are not even sure on (2) or (3), or anyone still has reservation on them, wouldn't it better to keep it as is (1)? Yes, particle_base will be messy because of the many functions from two_phase particles, but at least we get the speedup. Yes we might need to refactor in the future, but isn't that the assumption that we will refactor again? Lastly, if you are sure for (2), might as well go for (3) so we don't come back to it again later. So my vote will be (3) now!

WeijianLiang · 2020-07-23T17:38:24Z

@kks32 Thanks, I also vote for (2)

bodhinandach · 2020-07-24T21:52:28Z

After considering @ezrayst comment, I changed my vote to option (3). I know that the code will be slower, but the code is structurally way better and tidier. Also, if currently by moving to (3) causing us to be 30% slower than develop, I think we should rethink any other way to improve the speed. For instance, PR #677 has already improved the speed by 4% (thanks to @kks32). I think we can regain the current performance in short time by improving efficiency in other parts.

…/particles

yliang-sn · 2020-07-27T18:27:54Z

After talking with Ezra and Nanda, I think i will change to vote for 3 if we can improve the efficiency in other aspects.

…/particles

kks32 · 2020-07-28T17:36:21Z

Other containers like Abseil, folly, vector of vectors, static_vector, and small_vector implementations all gave slower or almost identical performance as the flat_map. Due to the 30% reduction in performance we are closing this issue. https://trello.com/c/fGF3MrKg/110-plan-for-next-week-notes

kks32 added 2 commits June 29, 2020 12:43

💻 Refactor particle to handle scalar and vector properties

f3f6fb2

🎯 Add testing to particle refactor of scalar properties

ad09dfa

kks32 added Priority: Critical Status: In progress Type: Refactor labels Jun 29, 2020

kks32 added this to the MPM Framework milestone Jun 29, 2020

kks32 requested review from srhgk2, WeijianLiang, ezrayst, cw646, tianchiTJ, bodhinandach and thiagordonho June 29, 2020 18:06

kks32 self-assigned this Jun 29, 2020

kks32 mentioned this pull request Jul 4, 2020

Feature/particle refactor #650

Closed

kks32 and others added 10 commits July 4, 2020 16:16

💻 Compute mass free function

e68ab89

Merge branch 'develop' of https://github.com/cb-geo/mpm into refactor…

d40b6b0

…/particles

🔧 modify mass, mass_density_volume

67303d8

🔧 modify compute_mass function

0a6ac16

🔧 refactor particle velocity and displacement

adfd93f

🔧 refactor nodal mass and volume

2b92b3f

🔧 refactor nodal velocity and acceleration

336cfe6

🔧 refactor nodal momentum, external, and internal forces

91d5c97

🔧 refactor nodal pressure

8ed2eb8

🔧 Add scalar and vector properties enum

02911bf

bodhinandach self-assigned this Jul 9, 2020

bodhinandach added 2 commits July 8, 2020 20:33

🔧 aesthetic clean up order of initialise()

0feaf6d

🔧 fix particle vector_properties_ size order

bcfb878

🚧 cosmetic changes as suggested

da1682e

🚧 add boolean property

3e1f9ff

bodhinandach added 2 commits July 22, 2020 15:54

Merge branch 'refactor/particles_properties' of https://github.com/cb…

a687fa8

…-geo/mpm into refactor/particles

🔧 modify function calls

47c5f48

Merge branch 'develop' into refactor/particles

6e2913b

Merge branch 'develop' of https://github.com/cb-geo/mpm into refactor…

4899191

…/particles

bodhinandach mentioned this pull request Jul 25, 2020

Semi-Implicit Navier-Stokes solver for incompressible flow #634

Open

bodhinandach added 4 commits July 24, 2020 19:45

🚧 🎯 add set_traction to boolean_properties and add testing

b8a3932

Merge branch 'develop' of https://github.com/cb-geo/mpm into refactor…

f2a982c

…/particles

🔧 fix particle_function for pressure smoothing

68506c1

🚧 🎯 refactor and add test for nodal boolean properties

0666b0a

bodhinandach changed the title ~~Refactor/particles~~ [Refactor] Particle variables and functions handling Jul 28, 2020

Merge branch 'develop' of https://github.com/cb-geo/mpm into refactor…

a0bbd15

…/particles

kks32 closed this Jul 28, 2020

kks32 deleted the refactor/particles branch July 28, 2020 22:06

kks32 mentioned this pull request Jul 28, 2020

Refactor particle to handle different types #637

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Refactor] Particle variables and functions handling #654

[Refactor] Particle variables and functions handling #654

kks32 commented Jun 29, 2020 •

edited

Loading

codecov bot commented Jun 29, 2020 •

edited

Loading

kks32 commented Jul 22, 2020 •

edited by bodhinandach

Loading

WeijianLiang commented Jul 22, 2020

kks32 commented Jul 22, 2020

yliang-sn commented Jul 22, 2020

kks32 commented Jul 22, 2020

yliang-sn commented Jul 22, 2020

kks32 commented Jul 22, 2020

kks32 commented Jul 23, 2020

ezrayst commented Jul 23, 2020 •

edited

Loading

WeijianLiang commented Jul 23, 2020

bodhinandach commented Jul 24, 2020 •

edited

Loading

yliang-sn commented Jul 27, 2020

kks32 commented Jul 28, 2020 •

edited

Loading

[Refactor] Particle variables and functions handling #654

[Refactor] Particle variables and functions handling #654

Conversation

kks32 commented Jun 29, 2020 • edited Loading

codecov bot commented Jun 29, 2020 • edited Loading

Codecov Report

kks32 commented Jul 22, 2020 • edited by bodhinandach Loading

WeijianLiang commented Jul 22, 2020

kks32 commented Jul 22, 2020

yliang-sn commented Jul 22, 2020

kks32 commented Jul 22, 2020

yliang-sn commented Jul 22, 2020

kks32 commented Jul 22, 2020

kks32 commented Jul 23, 2020

ezrayst commented Jul 23, 2020 • edited Loading

WeijianLiang commented Jul 23, 2020

bodhinandach commented Jul 24, 2020 • edited Loading

yliang-sn commented Jul 27, 2020

kks32 commented Jul 28, 2020 • edited Loading

kks32 commented Jun 29, 2020 •

edited

Loading

codecov bot commented Jun 29, 2020 •

edited

Loading

kks32 commented Jul 22, 2020 •

edited by bodhinandach

Loading

ezrayst commented Jul 23, 2020 •

edited

Loading

bodhinandach commented Jul 24, 2020 •

edited

Loading

kks32 commented Jul 28, 2020 •

edited

Loading