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

Add PrimitiveId data structure #1399

Closed
wants to merge 1 commit into from

Conversation

nilsvu
Copy link
Member

@nilsvu nilsvu commented Feb 27, 2019

Proposed changes

This data structure wraps a generic type T and provides a value() member function that allows it to be used by ObservationId.
I intend to replace my LinearSolver::IterationId with a PrimitiveId<size_t> and also use it as the type for the new NonlinearSolver::Tags::IterationId, as well as other components of the elliptic solver that need to keep track of a step number, such as AMR and Multigrid cycles.
I understand a PrimitiveId<double> is also useful for horizon finding and other components, so I'd appreciate some feedback. Putting this in as a draft PR for now to make sure we agree on the design before finalising this.

Types of changes:

  • New feature

Component:

  • Code

Code review checklist

  • The PR passes all checks, including unit tests, clang-tidy and IWYU.
    For instructions on how to perform the CI checks locally refer to the Dev
    guide on the Travis CI
    .
  • The code is documented and the documentation renders correctly. Run
    make doc to generate the documentation locally into BUILD_DIR/docs/html.
    Then open index.html.
  • The code follows the stylistic and code quality guidelines listed in the
    code review guide.

@kidder
Copy link
Member

kidder commented Feb 27, 2019

  1. I do not like the name as Primitive already has a meaning (Primitive vs Conservative)
  2. I do not see the motivation for this; why not just take IterationId outside of the LinearSolver namespace and reuse it. You could also add an increment operator to it if it is convenient

@wthrowe
Copy link
Member

wthrowe commented Feb 27, 2019

Why don't we just remove the .value() call from ObservationId and make people pass in a double? In the long term at least for evolutions I think all the observations are going to be identified by doubles anyway.

@nilsvu
Copy link
Member Author

nilsvu commented Feb 27, 2019

  • If I could pass a size_t or a double to ObservationId that would eliminate my need for this type. However @nilsdeppe didn’t like that if I understood him correctly.
  • Please suggest a better name @kidder, I couldn’t think of a better one
  • Moving IterationId out of the LinearSolver is precisely what this PR does. It makes the type generic so that it may be used by other code.

@kidder
Copy link
Member

kidder commented Feb 27, 2019

Why can't you use IterationId?

@wthrowe
Copy link
Member

wthrowe commented Feb 27, 2019

Could you just explicitly do static_cast<double> when you need an identifier rather than wrapping in a class that just exists to do that?

@nilsvu
Copy link
Member Author

nilsvu commented Feb 27, 2019

@kidder I can name this type IterationId, specialize it to size_t and place it in DataStructures, that’s fine with me. I was told that other code needs a similar type for double, which is why I made this generic. If ObservationId could take a size_t or double directly I wouldn’t need this type at all. Please make a decision and/or ping me to talk in a hangout

@nilsvu
Copy link
Member Author

nilsvu commented Feb 27, 2019

@wthrowe yes, only ObservationId needs a value() member function

@wthrowe
Copy link
Member

wthrowe commented Feb 27, 2019

But we could change that, as I suggested above.

@nilsdeppe
Copy link
Member

I'm pretty sure changing observationid to take a double is what we had actually agreed upon in a call with CalTech.

@nilsvu
Copy link
Member Author

nilsvu commented Feb 27, 2019

Yes let’s do that. It means I can use a plain size_t for the LinearSolver::Tags::IterationId and for similar tags.

@nilsdeppe
Copy link
Member

@markscheel would changing ObservationId in this manner make your life difficult right now?

@nilsvu
Copy link
Member Author

nilsvu commented Mar 4, 2019

Superseded by #1408

@nilsvu nilsvu closed this Mar 4, 2019
@nilsvu nilsvu deleted the primitive_id branch October 2, 2019 14:58
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.

None yet

4 participants