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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 5 commits into from
Feb 1, 2021
Merged

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

merged 5 commits into from
Feb 1, 2021

Conversation

hbrunie
Copy link
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
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
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
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.

src/program_options.hpp Outdated
@@ -250,6 +267,7 @@ class parser
std::string pde_str = DEFAULT_PDE_STR;
// pde to construct/evaluate
PDE_opts pde_choice = DEFAULT_PDE_OPT;
// pde selected case (f0)
Copy link
Collaborator

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
Collaborator Author

Choose a reason for hiding this comment

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

I forgot to remove ..

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
Collaborator

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
Collaborator Author

Choose a reason for hiding this comment

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

Yes.

src/tensors.hpp Outdated
vector<P, mem, resrc> &operator=(vector<P, mem, resrc> const &);

// move constructor/assignment (required to be same to same)
vector(vector<P, mem, resrc> &&);

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

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
Collaborator Author

Choose a reason for hiding this comment

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

OK.

Copy link
Collaborator Author

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.

@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