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

Refactor petsc examples to reduce redundant code #726

Merged
merged 7 commits into from
Apr 14, 2021

Conversation

jeremylt
Copy link
Member

@jeremylt jeremylt commented Apr 6, 2021

No description provided.

@jeremylt
Copy link
Member Author

jeremylt commented Apr 6, 2021

There is a small bug with BP3-6 in this that I need to track down

@jeremylt jeremylt force-pushed the jeremy/petsc-ex-build branch 2 times, most recently from 80bc9f7 to bcd4424 Compare April 6, 2021 03:36
@jeremylt
Copy link
Member Author

jeremylt commented Apr 6, 2021

Now there is just a bug with OCCA/OMP for bpssphere

@jeremylt
Copy link
Member Author

jeremylt commented Apr 6, 2021

That example runs fine in valgrind with the memcheck backends, so I suspect that we somehow have found another OCCA backend bug. I'll dig more tomorrow.

@jeremylt
Copy link
Member Author

jeremylt commented Apr 6, 2021

Nope, I was wrong. A missed 'takearray' in the original bpssphere

@jeremylt jeremylt changed the title WIP - refactor petsc examples to reduce redundant code Refactor petsc examples to reduce redundant code Apr 6, 2021
@jeremylt jeremylt requested a review from jedbrown April 6, 2021 05:21
@jeremylt jeremylt added 1-In Review and removed 0-WIP labels Apr 6, 2021
@jeremylt
Copy link
Member Author

jeremylt commented Apr 6, 2021

And we trimmed 781 lines of redundant code. We really should get around to doing this to the fluids miniapp one of these days.

I'll use this as the base to build the new BDDC example.

@jeremylt
Copy link
Member Author

jeremylt commented Apr 6, 2021

Mildly related - do we want to finally kill that meshes directory in this PR?

@jeremylt
Copy link
Member Author

jeremylt commented Apr 6, 2021

@LeilaGhaffari, just making sure you see this. I think we should do this with the fluids example too. I won't have time to get to it this month, but if you get a chance and/or are interested and hit any snags, let me know and I'll help.

@jeremylt
Copy link
Member Author

jeremylt commented Apr 6, 2021

@valeriabarra, I'll start the style guide PR after this PR is approved.

@LeilaGhaffari
Copy link
Member

@LeilaGhaffari, just making sure you see this. I think we should do this with the fluids example too. I won't have time to get to it this month, but if you get a chance and/or are interested and hit any snags, let me know and I'll help.

Yes, I am following this PR. I have used PetscFunctionList() for the fluids example and I was just able to get it work with the new release. I am hoping that the rest of the refactoring would be straightforward. Thanks for offering to help. I will let you know.

@jeremylt
Copy link
Member Author

jeremylt commented Apr 7, 2021

I went ahead and tried the new style in this PR so we could see if we like it.

@jeremylt jeremylt mentioned this pull request Apr 7, 2021
4 tasks
ceed_resource[PETSC_MAX_PATH_LEN] = "/cpu/self";
PetscInt l_size, g_size, xl_size,
q_extra = 1, // default number of extra quadrature points
ncomp_x = 3, // number of components of 3D physical coordinates
Copy link
Member

Choose a reason for hiding this comment

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

How about n_comp_x?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd be fine using num_comp_x (I think that's how we have it in Rust), but I'd prefer to avoid n_comp_x. The later feels harder to read to me.

Copy link
Member

Choose a reason for hiding this comment

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

I see your point. Yes, I like num_comp_x.

Copy link
Member Author

@jeremylt jeremylt Apr 8, 2021

Choose a reason for hiding this comment

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

Thanks for asking - I went ahead and made some changes in that vein

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for taking care of that. I will make the same changes in the fluids example.

@jeremylt
Copy link
Member Author

jeremylt commented Apr 8, 2021

I plan on squashing this to 3 commits before merging

  • code refactor
  • code style update
  • doc style update

@@ -78,7 +78,7 @@ struct RunParams_ {
MPI_Comm comm;
PetscBool test_mode, read_mesh, user_l_nodes, write_solution;
char *filename, *hostname;
PetscInt local_nodes, degree, q_extra, dim, num_comp_u, *m_elem;
PetscInt local_nodes, degree, q_extra, dim, num_comp_u, *mesh_elem;
Copy link
Member Author

Choose a reason for hiding this comment

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

Part of my philosophy with these name changes is that the PETSc (and solid mechanics) examples seem to be the entry point for people looking to understand and use libCEED (especially for new grad students), so I'm ok with slightly more verbose names that help ease that learning curve.

Copy link
Member

Choose a reason for hiding this comment

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

Very good point. This verbosity could have saved me a lot of time when I first started reading the fluids example.

Copy link
Member Author

Choose a reason for hiding this comment

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

Some of the pain with the fluids example comes from the fact I was the one who started it and it was my first attempt at making an mini-app :) hopefully this wave of style + modularity refactors will make things easier for everyone down the road

@jeremylt jeremylt mentioned this pull request Apr 9, 2021
Copy link
Member

@jedbrown jedbrown left a comment

Choose a reason for hiding this comment

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

Tests are passing and changes look reasonable at a glance, so I'm happy to merge.

@jeremylt jeremylt merged commit caa531a into main Apr 14, 2021
@jeremylt jeremylt deleted the jeremy/petsc-ex-build branch April 14, 2021 04:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants