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

EntropyReader Class #5341

Merged
merged 1 commit into from Aug 11, 2023
Merged

Conversation

RanpengLi
Copy link
Contributor

I opened up this pull request to test and fix some issues in my last pull request (#5334 Add EntropyReader Class). The previous one couldn't compile on the testers, but it could on my up-to-date local version. It also seems to have some strange indents.

@gassmoeller gassmoeller mentioned this pull request Aug 7, 2023
1 task
@RanpengLi RanpengLi changed the title [WIP] EntropyReader Class - fixing the compiling issue EntropyReader Class Aug 8, 2023
@RanpengLi
Copy link
Contributor Author

@gassmoeller Hi Rene, I have modified it according to your comments for #5334. This one doesn't have the strange indents in #5334.

@gassmoeller
Copy link
Member

/rebuild

Copy link
Member

@gassmoeller gassmoeller left a comment

Choose a reason for hiding this comment

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

Close, just a few comments left.


/**
* This class reads in an entropy-pressure material table and looks up material
* proporties for the given entropy and pressure.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* proporties for the given entropy and pressure.
* properties for the given entropy and pressure.

const std::string material_file_name);

/**
* Reads the specific heat for a given entropy and pressure.
Copy link
Member

Choose a reason for hiding this comment

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

Reads is maybe not the correct work. What about:

Suggested change
* Reads the specific heat for a given entropy and pressure.
* Returns the specific heat for a given entropy and pressure.

Same for all the other functions below.

EntropyReader::compressibility(const double entropy,
const double pressure) const
{
const double compressibility = material_lookup->get_data({entropy,pressure}, 2);
Copy link
Member

Choose a reason for hiding this comment

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

This seems not correct. The thermal_expansivity above is also listed as column 2. Which one is it?

@@ -29,7 +29,7 @@ namespace aspect
Command<dim>::execute (TableHandler &)
{
if (on_all_processes ||
(Utilities::MPI::this_mpi_process(this->get_mpi_communicator()) == 0))
Copy link
Member

Choose a reason for hiding this comment

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

Are these dealii:: prefixes here and in the other instances still necessary? I though the using statements had fixed that?

@RanpengLi
Copy link
Contributor Author

Thank you! I have addressed the comments and squashed the previous commits into one.

Copy link
Member

@gassmoeller gassmoeller left a comment

Choose a reason for hiding this comment

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

Looks good, thank you.

@gassmoeller gassmoeller merged commit 3192dc5 into geodynamics:main Aug 11, 2023
7 checks passed
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.

None yet

2 participants