Skip to content

New feature: Rename pde to fit with Matlab DG-SparseGrid#368

Merged
bmcdanie merged 5 commits intoproject-asgard:developfrom
hbrunie:renamepde
Feb 1, 2021
Merged

New feature: Rename pde to fit with Matlab DG-SparseGrid#368
bmcdanie merged 5 commits intoproject-asgard:developfrom
hbrunie:renamepde

Conversation

@hbrunie
Copy link
Copy Markdown
Collaborator

@hbrunie hbrunie commented Jan 18, 2021 •
edited
Loading

Proposed changes

This PR is here to solve the rename pde issue#344.
It involves adding a parameter to asgard binary.

  --case_selection <e.g. 1 (or 2)>      PDE case to solve; default is 1,
                                            sometimes there is a second case
                                            possible.

What type(s) of changes does this code introduce?

Put an x in the boxes that apply.

  • Bugfix
  • New feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

Does this introduce a breaking change?

  • Yes
  • No

What systems has this change been tested on?

Checklist

  • this PR is up to date with current the current state of 'develop'
  • code added or changed in the PR has been clang-formatted
  • this PR adds tests to cover any new code, or to catch a bug that is being fixed
  • documentation has been added (if appropriate)

@hbrunie hbrunie self-assigned this Jan 18, 2021
@hbrunie hbrunie added the hardening non bugfix/performance improvements label Jan 18, 2021
@hbrunie hbrunie linked an issue Jan 18, 2021 that may be closed by this pull request
renamed PDEs #344
Closed
@hbrunie hbrunie force-pushed the renamepde branch 10 times, most recently from dad5f7d to e306b0d Compare January 22, 2021 10:57
@hbrunie
Copy link
Copy Markdown
Collaborator Author

hbrunie commented Jan 22, 2021

I have rebased this PR above the branch of @bmcdanie fixing the time in analytic solution generation.
Once @bmcdanie PR would have been merged to asgard/develop I will have to rebase on develop and force push again.

@hbrunie hbrunie force-pushed the renamepde branch 3 times, most recently from 1f26603 to 27fa7fc Compare January 25, 2021 09:59
@hbrunie
Copy link
Copy Markdown
Collaborator Author

hbrunie commented Jan 25, 2021

To pass the tests coefficients and time_advance, we need the gold generated corresponding files for fokkerplanck1_pitch_E_case2 (gaussian) from the Matlab DG-SparseGrid repository.

This is not implemented yet. This is the corresponding [PR] (project-asgard/DG-SparseGrid#65) from this other repo.

@hbrunie hbrunie mentioned this pull request Jan 25, 2021
Open
@hbrunie
Rename pde fokkerplanck 4p1a and 4p2
7e813f4 
fokkerplanck1 from 4p1a to pitch E and
fokkerplanck1 from 4p2 to pitch C and
Change all 4p1a by pitch_E (into testing coefficients and time_advance
too).
clang format.
Fix a non-initialized variable warning in lib_dispatch.
@hbrunie
Adding fokkerplanck pitch E case 2.
9d143e7 
To add case we are using one more template value for PDE.
TODO: add case2 proper gold generated tests for coefficients and
time advance.
@hbrunie
Use macro to wrap assert (macro).
8fbddb4 
This fix the bug of not having real error root when assertion fails.
@hbrunie hbrunie requested a review from bmcdanie January 29, 2021 08:10
bmcdanie
bmcdanie requested changes Jan 29, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@bmcdanie bmcdanie left a comment

Choose a reason for hiding this comment

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

thanks for this PR! couple of minor notes, looks great overall.

Comment thread src/program_options.hpp Outdated
std::string pde_str = DEFAULT_PDE_STR;
// pde to construct/evaluate
PDE_opts pde_choice = DEFAULT_PDE_OPT;
// pde selected case (f0)
Copy link
Copy Markdown
Collaborator

@bmcdanie bmcdanie Jan 29, 2021

Choose a reason for hiding this comment

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

what does this comment refer to?

Copy link
Copy Markdown
Collaborator Author

@hbrunie hbrunie Feb 1, 2021

Choose a reason for hiding this comment

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

I forgot to remove ..

Comment thread src/program_options.hpp Outdated
static auto constexpr DEFAULT_PDE_STR = "continuity_2";
static auto constexpr DEFAULT_PDE_OPT = PDE_opts::continuity_2;
static auto constexpr DEFAULT_SOLVER = solve_opts::direct;
static auto constexpr DEFAULT_PDE_SELECTED_CASE = 0;
Copy link
Copy Markdown
Collaborator

@bmcdanie bmcdanie Jan 29, 2021

Choose a reason for hiding this comment

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

can you init this with the enum value instead of an integer?

Copy link
Copy Markdown
Collaborator Author

@hbrunie hbrunie Feb 1, 2021

Choose a reason for hiding this comment

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

Yes.

Comment thread src/tensors.hpp Outdated
vector(vector<P, mem, resrc> &&);

// as with copy assignment, static tools::expect added
// as with copy assignment, static expect added
Copy link
Copy Markdown
Collaborator

@bmcdanie bmcdanie Jan 29, 2021

Choose a reason for hiding this comment

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

I believe this is my mistake originally - should say "static assert" but my sed changes to "tools::expect", then your sed changed that to "static expect".

Copy link
Copy Markdown
Collaborator Author

@hbrunie hbrunie Feb 1, 2021

Choose a reason for hiding this comment

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

OK.

Copy link
Copy Markdown
Collaborator Author

@hbrunie hbrunie Feb 1, 2021

Choose a reason for hiding this comment

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

I also changed the one line 153.

Comment thread src/tensors.hpp Outdated
Comment thread src/pde/pde_fokkerplanck1_pitch_E.hpp Outdated
@hbrunie
Requested changes to PR.
c091acc 
Remove obsolete comment.
Enum instead of int.
static expect -> static assert
brackets for one line if/else.
@bmcdanie bmcdanie merged commit 8e6e0d4 into project-asgard:develop Feb 1, 2021
@hbrunie hbrunie deleted the renamepde branch March 8, 2021 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bmcdanie bmcdanie bmcdanie requested changes

Assignees

@hbrunie hbrunie

Labels

hardening non bugfix/performance improvements

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

renamed PDEs

2 participants

@hbrunie @bmcdanie