-
Notifications
You must be signed in to change notification settings - Fork 19
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
Minor suggestions for readability. #51
Conversation
57b176c
to
a6d2b34
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maartenarnst thanks for your thorough review of the manuscript! I mostly agree with the proposed changes and have only left a few comments and even fewer requests for changes.
discretization.tex
Outdated
The most intrusive approach is to build the application around the Panzer package, which provides all of the above, plus the handling of linear and nonlinear solvers and integrated constrained optimization capabilities. | ||
In the following we describe some of the Trilinos discretization packages. For brevity, we do not include the description of these packages: Sierra ToolKit (STK), Krino and Percept, which provide mesh and level-set tools, and Shards, which provides tools for mesh topology. We refer to Trilinos website (\url{https://trilinos.github.io}) for a brief description of these packages. | ||
In the following we describe some of the Trilinos discretization packages. For brevity, we do not include the description of the following packages: Sierra ToolKit (STK), Krino and Percept, which provide mesh and level-set tools, and Shards, which provides tools for mesh cell topology. We refer to the Trilinos website (\url{https://trilinos.github.io}) for a brief description of these packages. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far, we do not explain why we omit explanations. This modification attempts to explain the reason:
In the following we describe some of the Trilinos discretization packages. For brevity, we do not include the description of the following packages: Sierra ToolKit (STK), Krino and Percept, which provide mesh and level-set tools, and Shards, which provides tools for mesh cell topology. We refer to the Trilinos website (\url{https://trilinos.github.io}) for a brief description of these packages. | |
In the following we describe Trilinos' native discretization packages. For brevity, we do not include the description of the packages that are snapshotted into Trilinos such as Sierra ToolKit (STK), Krino and Percept, which provide mesh and level-set tools, and Shards, which provides tools for mesh cell topology. We refer to the Trilinos website (\url{https://trilinos.github.io}) for a brief description of these packages. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about the word "native". I've merged the first and the second sentence to avoid the issue. Also, I'm not sure whether Percept
is snapshotted. It doesn't seem to be the case in the repository (?). So I've also reformulated that sentence a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cgcgcg @ccober6 @mmwolf can you find out about the state of Percept? Its public GitHub repository appears to be archived (https://github.com/sandialabs/percept), but I don't know if there's an Sandia-internal development repository. Is Percept still actively developed? If yes, where? If not, should we remove it from the paper?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Percept is developed in Trilinos as far a I know. @mperego ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, Percept is not developed in Trilinos. It's also no longer developed and we will remove it from Trilinos as soon as we replace its capability with a new mesh refinement tool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we make a separate PR only on this matter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@searhein I am already on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mayrmt I think it's fine if you do not mention Percept at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Mauro. We should leave off packages that we know will be deprecated relatively soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
d355088
to
c90329d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cgcgcg I am fine with the proposed changes. Are you?
c90329d
to
c062c29
Compare
@maartenarnst Thanks for the changes! I can also take a look after this week. I'm on vacation this week. But if everyone is happy with it, please merge it 👍 |
Looks like this PR is ready to be merged. The only open point (snapshotted packages) will be addressed in a separate PR. I'll merge now. |
This PR proposes minor suggestions for readability.