Skip to content

Feature/xreg2#207

Merged
Algiane merged 5 commits intoMmgTools:developfrom
corentin-prigent:feature/xreg2
Mar 20, 2023
Merged

Feature/xreg2#207
Algiane merged 5 commits intoMmgTools:developfrom
corentin-prigent:feature/xreg2

Conversation

@corentin-prigent
Copy link
Copy Markdown
Contributor

Added the possibility of choosing a value val for the regularization of coordinates when using option -xreg val :
Let P be a point to regularize. Let Q be the mean of surrounding points on manifold. New point R is computed as:
R = val * Q + (1 - val) * P
Default value is val = 0.4. Given value should be comprised between 0 and 1.

One CI test for each software has been added as well.

@corentin-prigent corentin-prigent changed the base branch from master to develop March 3, 2023 17:52
@Algiane Algiane added the kind: enhancement enhancement to an existing feature label Mar 3, 2023
Copy link
Copy Markdown
Member

@Algiane Algiane left a comment

Choose a reason for hiding this comment

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

Hi,
Thanks for this PR. I have few questions an the variable name choice and the value of coefficients.

MMG5_pPar par;
double dhd,hmin,hmax,hsiz,hgrad,hgradreq,hausd;
double min[3],max[3],delta,ls,rmc;
double min[3],max[3],delta,ls,lxreg,rmc;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why have you name the variable lxreg and not xreg?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There already is a attribute of struct info that is called xreg: an integer to tell whether the option is activated or not. Hence, xreg is always an integer, lxreg is always a double.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Then, in other occurences, lxreg is used to keep consistency, even if there would be no conflict with the name xreg.

#define MMG5_LAG -1 /**< default value for lagrangian option */
#define MMG5_NR -1 /**< no ridge detection */
#define MMG5_LS 0.0 /**< default level-set to discretize */
#define MMG5_LXREG 0.4 /**< default relaxation parameter for coordinate regularization */
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why have you name the variable lxreg and not xreg?

MMG3D_DPARAM_hgrad, /*!< [val], Control gradation */
MMG3D_DPARAM_hgradreq, /*!< [val], Control gradation on required entites (advanced usage) */
MMG3D_DPARAM_ls, /*!< [val], Value of level-set */
MMG3D_DPARAM_lxreg, /*!< [val], Value of relaxation parameter for coordinates regularization (0<val<1) */
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why have you name the variable lxreg and not xreg?

Comment on lines +527 to +528
lm1 = 0.4;
lm2 = 0.399;
lm1 = mesh->info.lxreg;
lm2 = lm1;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess that the slight difference between the lm1 and lm2 values is here to avoid cases of oscillations during regularization... Is it possible to check that (by reading the code because it may be hard and quite random to reproduce oscillations)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

To avoid oscillations, i changed the computation of lm2 so that it is always slightly lower than lm1 .

@Algiane Algiane merged commit 65798da into MmgTools:develop Mar 20, 2023
@Algiane
Copy link
Copy Markdown
Member

Algiane commented Mar 20, 2023

Thanks for this improvement.

@corentin-prigent corentin-prigent deleted the feature/xreg2 branch March 20, 2023 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind: enhancement enhancement to an existing feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants