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

EnzoInitialShuCollapse parameter refactorization #388

Conversation

jakereinheimer
Copy link
Contributor

Pull request summary

Refactoring EnzoInitialShuCollapse parameters. Addresses issue #383. Short because I am going through it live with Matthew

@jwise77
Copy link
Contributor

jwise77 commented May 15, 2024

There are some compilation errors

/home/circleci/enzo-e/src/Enzo/initial/EnzoInitialShuCollapse.cpp:21:4: error: expected ‘{’ before ‘center_’
   21 |    center_{}
      |    ^~~~~~~
/home/circleci/enzo-e/src/Enzo/initial/EnzoInitialShuCollapse.cpp: At global scope:
/home/circleci/enzo-e/src/Enzo/initial/EnzoInitialShuCollapse.cpp:21:4: error: ‘center_’ does not name a type
/home/circleci/enzo-e/src/Enzo/initial/EnzoInitialShuCollapse.cpp:22:4: error: ‘drift_velocity_’ does not name a type
   22 |    drift_velocity_{}
      |    ^~~~~~~~~~~~~~~
/home/circleci/enzo-e/src/Enzo/initial/EnzoInitialShuCollapse.cpp:23:1: error: expected unqualified-id before ‘{’ token
   23 | {
      | ^

Copy link
Contributor

@mabruzzo mabruzzo left a comment

Choose a reason for hiding this comment

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

This is fantastic! I really appreciate you doing this!

I highlighted 2 missing commas that are needed in order to get this to compile properly.

There are also 2 other changes that are necessary:

  1. The order in which the members appear in the "member-initializer-list" in the constructors should match the order in which they are declared within the class itself (jump to the end of EnzoInitialShuCollapse to see this ordering).
    • technically, this is not strictly necessary, but it's good practice (if you don't do this, you can get unexpected behavior while using one member to initialize another member)
    • plus g++ raises some annoying warnings if you don't do this
  2. More importantly, you are changing the default value of the parameters
    • this is my fault for not explaining this.
    • When you call something like p.value_float("truncation_radius", ...) the trailing value represents the default value.
    • Currently, you've changed the default values compared to what was previously used within EnzoConfig::read_initial_shu_collapse_

src/Enzo/initial/EnzoInitialShuCollapse.cpp Outdated Show resolved Hide resolved
src/Enzo/initial/EnzoInitialShuCollapse.cpp Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants