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.

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

amrex::Parser #2063

Merged
merged 7 commits into from
Jul 14, 2021
Merged

amrex::Parser #2063

merged 7 commits into from
Jul 14, 2021

Conversation

WeiqunZhang
Copy link
Member

Replace WarpXParser with amrex::Parser. Roundoff errors are expected because
of additional optimization in amrex::Parser.

@lgtm-com

This comment has been minimized.

@lgtm-com

This comment has been minimized.

@lgtm-com

This comment has been minimized.

@lgtm-com

This comment has been minimized.

@WeiqunZhang WeiqunZhang force-pushed the amrex_parser branch 2 times, most recently from c05226a to 0d8eecd Compare July 10, 2021 22:29
@WeiqunZhang WeiqunZhang changed the title [WIP] amrex::Parser amrex::Parser Jul 10, 2021
@WeiqunZhang WeiqunZhang requested review from lucafedeli88 and ax3l July 10, 2021 23:39
@ax3l ax3l self-assigned this Jul 12, 2021
@ax3l ax3l added component: core Core WarpX functionality component: third party Changes in WarpX that reflect a change in a third-party library labels Jul 12, 2021
Replace WarpXParser with amrex::Parser. Roundoff errors are expected because
of additional optimization in amrex::Parser.
@lgtm-com

This comment has been minimized.

@lgtm-com

This comment has been minimized.

Copy link
Member

@ax3l ax3l left a comment

Choose a reason for hiding this comment

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

Thank you for generalizing this! Thanks also for updating the docs.
Awesome that this also resolved the recursive function usage. Also looking forward to the temporary variable support with r=sqrt(x*x+z*x); ... expressions 🎉 Do you want to add this in one of the tests?

I quickly checked if #1874 still works and it looks like it's covered in CI.

Added minor inline comments, looks already great to me.

@WeiqunZhang
Copy link
Member Author

Maybe we can add the temporary variable to tests later.

@WeiqunZhang
Copy link
Member Author

As @dpgrote requested, integer is supported in amerx::IParser. The main difference is how division works. Dave will use it to parse integer parameters in a future PR.

@WeiqunZhang
Copy link
Member Author

Not sure why the DPC++ build fails now. 🤔

@WeiqunZhang
Copy link
Member Author

Looks like the issue is Source/Particles/Collision/BackgroundMCCCollision.cpp in 437a53f.

@lgtm-com

This comment has been minimized.

@WeiqunZhang
Copy link
Member Author

The issue is MCCProcess is not trivially copyable because it has a ManagedVector member. I think we should try to fix this in another PR.

@WeiqunZhang
Copy link
Member Author

The MCCProcess issue is addressed in #2085.

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jul 14, 2021

This pull request fixes 1 alert when merging e3ca37a into 40e36e1 - view on LGTM.com

fixed alerts:

  • 1 for Use of goto

@@ -127,7 +127,7 @@ public:
}

// get parser
HostDeviceParser<m_nvars> reduction_function_parser = getParser(m_parser);
auto reduction_function_parser = m_parser->compile<m_nvars>();
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering if the many specializations in various TUs impact compile-time.
Will take a look when CI ran through just to get an idea.

Copy link
Member

@ax3l ax3l Jul 14, 2021

Choose a reason for hiding this comment

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

Haha, the error bar in CI is about 5min wide, thus it's hard to say. Looks still the same, given that fluctuation :D

@ax3l ax3l merged commit ffb3bb8 into ECP-WarpX:development Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: core Core WarpX functionality component: third party Changes in WarpX that reflect a change in a third-party library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants