Skip to content

Conversation

@majosm
Copy link
Collaborator

@majosm majosm commented Sep 26, 2022

Doesn't need to be on a branch anymore now that inducer/grudge#280 is merged.

Questions for the review:

  • Is the scope and purpose of the PR clear?
    • The PR should have a description.
    • The PR should have a guide if needed (e.g., an ordering).
  • Is every top-level method and class documented? Are things that should be documented actually so?
  • Is the interface understandable? (I.e. can someone figure out what stuff does?) Is it well-defined?
  • Does the implementation do what the docstring claims?
  • Is everything that is implemented covered by tests?
  • Do you see any immediate risks or performance disadvantages with the design? Example: what do interface normals attach to?

@matthiasdiener
Copy link
Member

matthiasdiener commented Sep 26, 2022

should this point to Kaushik's fork?

@majosm
Copy link
Collaborator Author

majosm commented Sep 26, 2022

should this point to Kaushik's fork?

Dunno, it's the same as Andreas' right now (or at least it was before the multi-volume stuff went in).

@kaushikcfd Are you planning to add any fusion stuff to your grudge in the near future?

@majosm majosm force-pushed the restore-grudge-main branch from 6401121 to e05aa34 Compare September 26, 2022 15:46
@majosm majosm marked this pull request as ready for review September 26, 2022 19:38
@majosm
Copy link
Collaborator Author

majosm commented Sep 26, 2022

I think we can go ahead with this as is and switch it over to Kaushik's if/when needed.

@matthiasdiener matthiasdiener enabled auto-merge (squash) September 26, 2022 22:45
@matthiasdiener matthiasdiener merged commit d213de9 into illinois-ceesd:main Sep 26, 2022
@kaushikcfd
Copy link
Collaborator

Are you planning to add any fusion stuff to your grudge in the near future?

No. I think grudge is feature complete with inducer/grudge#260.

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