CEED / Laghos

High-order Lagrangian Hydrodynamics Miniapp
http://ceed.exascaleproject.org/miniapps
BSD 2-Clause "Simplified" License
187 stars 60 forks source link

ROM README update #120

Closed dylan-copeland closed 3 years ago

dylan-copeland commented 3 years ago

The README.md file in the rom directory is updated with specific information about the ROM version of Laghos.

kevinhkhuynh commented 3 years ago

Do we want users to be able to use the main Laghos directories versions of metis and hypre? Or do we want to force them to use the same libraries as the paper? We currently use ParMetis 4.0.3 and Hypre 2.11.2 for our results.

dylan-copeland commented 3 years ago

@kevinhkhuynh I expect different versions of metis and hypre to have a small effect on results, on the level of round-off error (1e-12 or less). I don't see a reason to enforce a certain version.

kevinhkhuynh commented 3 years ago

@kevinhkhuynh I expect different versions of metis and hypre to have a small effect on results, on the level of round-off error (1e-12 or less). I don't see a reason to enforce a certain version.

Sounds good to me. In that case, in another PR, I'll have the makefile look to ../../ for any libraries first, then dependencies second so users can use their main Laghos branch libraries that they've installed.

chldkdtn commented 3 years ago

@dylan-copeland didn't we get a very different result in our ROM when we do not use metis, for example, not ParMetis, in the past? Do you remember?

dylan-copeland commented 3 years ago

@chldkdtn This came up in the discussion about PR 119. The regression tests failed, because they compared basis values. The difference that metis vs. parmetis would make would be in the partitioning of the spatial domain in parallel, which should have round-off level differences but different DOF definitions. It is not possible to compare solution or basis values, with different DOF definitions and different numbers of DOFs on each process. However, the accuracy should have very little change.

chldkdtn commented 3 years ago

@dylan-copeland I was not talking about PR 119. Very beginning of this project when only you and I were working on it, I recall that if we use metis, not parmetis, we get very different solutions. It was definitely at the stage where our ROM for Laghos was premature, but it just came to my mind.

dylan-copeland commented 3 years ago

@chldkdtn I don't recall the details of that issue. Hopefully, we just saw a different DOF ordering. The important question is whether the difference is just in the DOF indices and parallel partitioning, or the solution accuracy. That is, we should compare L2 norms of differences (numerically integrated), not l2 norms (indices are reordered). This could be answered with the current state of the code by running our tests with metis and parmetis, to see if there is a difference in the L2 norm of the error (comparing FOM solutions and ROM errors). This is what the absolute regression tests do, so maybe they could be run with different builds to compare results.

chldkdtn commented 3 years ago

@dylan-copeland Thank you for editing "Code Structure" section. That looks good. Would you also edit Sections, "Purpose," "Characteristics?" Both sections are tailored for Laghos, not for Laghos ROM.

chldkdtn commented 3 years ago

@dylan-copeland @kevinhkhuynh @siuwuncheung I have introduced the term, "LaghosROM," to refer "the ROM version of Laghos." Let's use this terminology throughout this rom/README.md file.

dylan-copeland commented 3 years ago

@siuwuncheung The plot files added in the last commit dc77df0432b0579ccf79224ab5c4da2bbcb09ccc are somewhat large. By themselves, they are not very bad, but if we plan to put lots of plots in the README in the future it could make the Laghos repo large. Maybe we should consider whether plot files should be in the repo in general. @vladotomov @tzanio do you have some guidelines for us on limiting large files like these?

vladotomov commented 3 years ago

There aren't strict rules for Laghos, but these are good to remember:

chldkdtn commented 3 years ago

@tzanio I guess that the copyright of CEED in README file of rom subdirectory must stay there although the rom work is not supported by CEED. What do you think, @tzanio ?

tzanio commented 3 years ago

I guess that the copyright of CEED in README file of rom subdirectory must stay there although the rom work is not supported by CEED

No, I think you should use a simpler README, like the one in the serial/ directory

dylan-copeland commented 3 years ago

It may be a better idea to have plots and other presentation materials on a webpage, not in the Laghos repo. That way, we could keep the README file size small, with maybe a link to a webpage outside the repo, like mfem.org. The README could report error norms instead of showing plots, or even that could just go in an external webpage. Plots are nice for presentation, but I don't think they are very essential here, as the main point is that the error is small. Since results will change over time, and more results will be added in the future, it may not be very manageable to have it all contained in the Laghos repo.

In any case, since some plot files were changed in at least one commit, I suppose we should squash merge (whether we keep the plot files or not).

chldkdtn commented 3 years ago

Hi, @kevinhkhuynh, when you get a chance, would you review and approve?