Handle the deprecated parameters of individual entries to a list entry #1171

acdaigneault · 2024-06-12T23:16:57Z

Description

Parameters are often changed as refactoring in the DEM solver, but it causes issues when running simulation with old parameter files.
Those files get incompatible with the latest version of the code and need to be updated by the user.
There is a function in deal.II (called declare_alias()) that allows to still use old parameters as alias, but need to be the same type so the parsing stays the same. Also, it can give a warning about the deprecated file.
We started, and we are planning, to convert some old individual parameter entries to one parameter that is a list of values.
It means 3 parameters of double will be changed to one parameter of a list of double.
In order to avoid the above, this refactoring aims to handles those old parameters.
The only issue with the current implementation is there is no way to assert if the new parameter is set as a double.
Also, this PR applies those changes to the gravity force in DEM.

Testing

New test deprecated_parameter uses the old gx, gy, gz parameters.
The old parameters (in 2d or 3d) were tested and it works well and gives a warning for the deprecated parameter.

Documentation

The parameters gx, gy and gz are now g.

Miscellaneous (will be removed when merged)

I suggest we update the same test if we use other deprecated parameters
I changed the g parameter because I wanted to add a function version for my example, but I realised that it won't be very effective. So I drop this idea, but kept the list parameter. I added a way to use deprecated parameters because the gravity parameter is in every DEM prm file and the change may annoyed people (meeee the first!)

Checklist (will be removed when merged)

See this page for more information about the pull request process.

Code related list:

All in-code documentation related to this PR is up to date (Doxygen format)
Lethe documentation is up to date
The branch is rebased onto master
Code is indented with indent-all and .prm files (examples and tests) with prm-indent

Pull request related list:

No other PR is open related to this refactoring
Labels are applied
There are at least 2 reviewers (or 1 if small feature) excluding the responsible for the merge
If this PR closes an issue or is related to a project, it is linked in the "Projects" or "Development" section
If any future works is planed, an issue is opened
The PR description is cleaned and ready for merge

blaisb

Just two small things where a g conversion is missing, otherwise very good. Love this. Maybe a good idea to add a changelog entry?

blaisb · 2024-06-13T12:08:15Z

applications_tests/lethe-particles/depreciated_parameters.output

@@ -0,0 +1,65 @@
+Warning in file <depreciated_parameters.prm>: You are using the deprecated spelling <gx> of the parameter <g>.


God this is fucking nice

doc/source/parameters/dem/lagrangian_physical_properties.rst

...ved-cfd-dem/gas-solid-spouted-cylinder-bed/dem-packing-in-spouted-cylinder-bed-generator.prm

examples/unresolved-cfd-dem/gas-solid-spouted-cylinder-bed/gas-solid-spouted-cylinder-bed.prm

include/core/parameters.h

voferreira

Amazing way to handle deprecated parameters. Good job! I added some tiny comments here and there, and I believe you are aware of the "deprecated" instead of "depreciated". Apart from that, good to go for me.

applications_tests/lethe-particles/depreciated_parameters.output

applications_tests/lethe-particles/depreciated_parameters.prm

voferreira · 2024-06-13T14:26:51Z

applications_tests/lethe-particles/periodic_boundary_box.prm

-  set gx                       = 0.0
-  set gy                       = -1
-  set gz                       = 0.0
+  set g                        = 0.0, -1, 0.0


Weird value for the gravity haha

voferreira · 2024-06-13T14:27:18Z

applications_tests/lethe-particles/periodic_boundary_load_balancing.prm

-  set gx                       = 0.0
-  set gy                       = -1
-  set gz                       = 0.0
+  set g                        = 0.0, -1, 0.0


Why this gravity?

Values of test don't need to be realistic. Sometime, (let's speak for myself) we need to tuned parameter in order to reproduce a specific bug or to reduce simulation time

voferreira · 2024-06-13T14:28:14Z

doc/source/parameters/dem/lagrangian_physical_properties.rst

-    # Gravitational acceleration in z direction
-    set gz                       = -9.81
+    # Gravitational acceleration vector
+    set g                       = 0.0, 0.0, 0.0


Did you change the default value?

No it was always set to 0.0 for all directions in the code, the doc was wrong

include/core/parameters.h

source/core/parameters_lagrangian.cc

acdaigneault · 2024-06-13T14:50:34Z

@blaisb Above the change log entry, I will update the PR template. Do you prefer I do it in this PR or in another very small PR, let's say without other reviewers?

blaisb · 2024-06-13T15:35:52Z

@blaisb Above the change log entry, I will update the PR template. Do you prefer I do it in this PR or in another very small PR, let's say without other reviewers?

Whatever you think is best, both for me are OK. I generally prefer seperated PRs, but if its really minor, go ahead and do it in this one.

blaisb

All good to go for me. Just add the flag reviewed and ready to merge once @OGaboriault is fine with the PR.

#1171) Description Parameters are often changed as refactoring in the DEM solver, but it causes issues when running simulation with old parameter files. Those files get incompatible with the latest version of the code and need to be updated by the user. There is a function in deal.II (called declare_alias()) that allows to still use old parameters as alias, but need to be the same type so the parsing stays the same. Also, it can give a warning about the deprecated file. We started, and we are planning, to convert some old individual parameter entries to one parameter that is a list of values. It means 3 parameters of double will be changed to one parameter of a list of double. In order to avoid the above, this refactoring aims to handles those old parameters. The only issue with the current implementation is there is no way to assert if the new parameter is set as a double. Also, this PR applies those changes to the gravity force in DEM. Testing New test deprecated_parameter uses the old gx, gy, gz parameters. The old parameters (in 2d or 3d) were tested and it works well and gives a warning for the deprecated parameter. Documentation The parameters gx, gy and gz are now g.

chaos-polymtl#1171) Description Parameters are often changed as refactoring in the DEM solver, but it causes issues when running simulation with old parameter files. Those files get incompatible with the latest version of the code and need to be updated by the user. There is a function in deal.II (called declare_alias()) that allows to still use old parameters as alias, but need to be the same type so the parsing stays the same. Also, it can give a warning about the deprecated file. We started, and we are planning, to convert some old individual parameter entries to one parameter that is a list of values. It means 3 parameters of double will be changed to one parameter of a list of double. In order to avoid the above, this refactoring aims to handles those old parameters. The only issue with the current implementation is there is no way to assert if the new parameter is set as a double. Also, this PR applies those changes to the gravity force in DEM. Testing New test deprecated_parameter uses the old gx, gy, gz parameters. The old parameters (in 2d or 3d) were tested and it works well and gives a warning for the deprecated parameter. Documentation The parameters gx, gy and gz are now g. Former-commit-id: 6c4a9d0

#1171) Description Parameters are often changed as refactoring in the DEM solver, but it causes issues when running simulation with old parameter files. Those files get incompatible with the latest version of the code and need to be updated by the user. There is a function in deal.II (called declare_alias()) that allows to still use old parameters as alias, but need to be the same type so the parsing stays the same. Also, it can give a warning about the deprecated file. We started, and we are planning, to convert some old individual parameter entries to one parameter that is a list of values. It means 3 parameters of double will be changed to one parameter of a list of double. In order to avoid the above, this refactoring aims to handles those old parameters. The only issue with the current implementation is there is no way to assert if the new parameter is set as a double. Also, this PR applies those changes to the gravity force in DEM. Testing New test deprecated_parameter uses the old gx, gy, gz parameters. The old parameters (in 2d or 3d) were tested and it works well and gives a warning for the deprecated parameter. Documentation The parameters gx, gy and gz are now g. Former-commit-id: 6c4a9d0

acdaigneault added Maintenance Ready for review Refactoring This PR is only refactoring or clean up Help wanted Extra attention is needed and removed Ready for review labels Jun 12, 2024

acdaigneault requested review from voferreira and blaisb June 13, 2024 01:33

acdaigneault removed the Help wanted Extra attention is needed label Jun 13, 2024

acdaigneault changed the title ~~Handle the depreciated parameters of individual entries to a list entry~~ Handle the deprecated parameters of individual entries to a list entry Jun 13, 2024

blaisb approved these changes Jun 13, 2024

View reviewed changes

voferreira approved these changes Jun 13, 2024

View reviewed changes

acdaigneault added 7 commits June 13, 2024 11:11

Modify the g parameter from gx, gy, gx to g list

a69a369

Update .prm

d187c74

Add a handling of list parameters with depreciated individual parameters

6068cce

Add more encapsulated version

7b9fa62

Documentation for Doxygen

915d87d

Add test and documentation

726cf40

Correct "depreciated" to "deprecated"

587688e

acdaigneault added 2 commits June 13, 2024 11:56

Fix some prm after modifications and postprocess scripts

abf6465

Modify parameter for examples in documentation

ac2c23f

acdaigneault force-pushed the dem_modify-g-parameter branch from fdc3f94 to ac2c23f Compare June 13, 2024 16:03

acdaigneault added 2 commits June 13, 2024 12:11

Apply suggestions from code review

53c3fd6

Add change log

23b250a

acdaigneault requested a review from OGaboriault June 13, 2024 17:37

Update PR templates

cf6d92e

blaisb approved these changes Jun 13, 2024

View reviewed changes

acdaigneault added the Reviewed and ready to merge label Jun 13, 2024

blaisb merged commit 6c4a9d0 into master Jun 14, 2024
8 checks passed

blaisb deleted the dem_modify-g-parameter branch June 14, 2024 12:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle the deprecated parameters of individual entries to a list entry #1171

Handle the deprecated parameters of individual entries to a list entry #1171

acdaigneault commented Jun 12, 2024 •

edited

Loading

blaisb left a comment •

edited

Loading

blaisb Jun 13, 2024

voferreira left a comment

voferreira Jun 13, 2024

voferreira Jun 13, 2024

acdaigneault Jun 13, 2024

voferreira Jun 13, 2024

acdaigneault Jun 13, 2024

acdaigneault commented Jun 13, 2024

blaisb commented Jun 13, 2024

blaisb left a comment

		@@ -0,0 +1,65 @@
		Warning in file <depreciated_parameters.prm>: You are using the deprecated spelling <gx> of the parameter <g>.

Handle the deprecated parameters of individual entries to a list entry #1171

Handle the deprecated parameters of individual entries to a list entry #1171

Conversation

acdaigneault commented Jun 12, 2024 • edited Loading

Description

Testing

Documentation

Miscellaneous (will be removed when merged)

Checklist (will be removed when merged)

blaisb left a comment • edited Loading

Choose a reason for hiding this comment

blaisb Jun 13, 2024

Choose a reason for hiding this comment

voferreira left a comment

Choose a reason for hiding this comment

voferreira Jun 13, 2024

Choose a reason for hiding this comment

voferreira Jun 13, 2024

Choose a reason for hiding this comment

acdaigneault Jun 13, 2024

Choose a reason for hiding this comment

voferreira Jun 13, 2024

Choose a reason for hiding this comment

acdaigneault Jun 13, 2024

Choose a reason for hiding this comment

acdaigneault commented Jun 13, 2024

blaisb commented Jun 13, 2024

blaisb left a comment

Choose a reason for hiding this comment

acdaigneault commented Jun 12, 2024 •

edited

Loading

blaisb left a comment •

edited

Loading