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

Fix VTK output to put plot files under output path #1343

Merged
merged 2 commits into from
Mar 5, 2021

Conversation

klevzoff
Copy link
Contributor

@klevzoff klevzoff commented Mar 3, 2021

Resolves #1342

Collateral "damage" is mostly due to some code cleanup related to path handling.

Rebaseline PR: https://github.com/GEOSX/integratedTests/pull/121
(only for tests using VTK output)

@klevzoff klevzoff added type: bug Something isn't working effort: 1 day labels Mar 3, 2021
@klevzoff klevzoff self-assigned this Mar 3, 2021
Comment on lines +493 to +496
if( m_previousCycle == -1 )
{
makeDirsForPath( joinPath( m_outputDir, m_outputName ) );
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The VTK output folder is now created on first write as opposed to writer object creation time, because the name of the directory is set after reading the XML input.

@@ -25,12 +25,12 @@ namespace geosx
{

///////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
void getAbsolutePath( std::string const & path, std::string & absolutePath )
std::string getAbsolutePath( std::string const & path )
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want to use geosx::string and not std::string (even though it's the same right now).

Copy link
Contributor Author

@klevzoff klevzoff Mar 4, 2021

Choose a reason for hiding this comment

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

Yes... but there's a circular inclusion problem that I didn't want to touch in this PR. Namely, DataTypes.hpp needs to include Path.hpp in order to provide some related typedefs and lambda dispatch for Path type. On the other hand, Path.hpp would need to include DataTypes.hpp in order to gain access to geosx::string.

One solution is to split a small portion of DataTypes.hpp into a separate header, BasicTypes.hpp, that will provide aliases like localIndex, globalIndex, real64, string. It can then be independently included in Path.hpp and DataTypes.hpp. I didn't do it here as it would create merge conflicts for the BoomerAMG PR that is about to be merged, but we can address this subsequently.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok then

Copy link
Member

Choose a reason for hiding this comment

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

It is intentional that we kept std::string here. Path is its own type derived exclusively from std::string.

@klevzoff klevzoff force-pushed the bugfix/klevzoff/vtk-output-dir branch from 59d322c to aef2148 Compare March 4, 2021 05:50
@klevzoff klevzoff force-pushed the bugfix/klevzoff/vtk-output-dir branch from aef2148 to fd98a25 Compare March 4, 2021 19:40
@klevzoff klevzoff requested a review from rrsettgast March 4, 2021 19:53
@@ -25,12 +25,12 @@ namespace geosx
{

///////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
void getAbsolutePath( std::string const & path, std::string & absolutePath )
std::string getAbsolutePath( std::string const & path )
Copy link
Member

Choose a reason for hiding this comment

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

It is intentional that we kept std::string here. Path is its own type derived exclusively from std::string.

*/
void getAbsolutePath( std::string const & path, std::string & absolutePath );
std::string getAbsolutePath( std::string const & path );
Copy link
Member

Choose a reason for hiding this comment

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

Re: string vs std::string. Here we would be passing string as the argument, so it should have a UDC to std::string, or be an alias for std::string.

VTKPVDWriter::VTKPVDWriter( string const & fileName ):
m_fileName( fileName )
VTKPVDWriter::VTKPVDWriter( string fileName ):
m_fileName( std::move( fileName ) )
Copy link
Member

Choose a reason for hiding this comment

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

why the move?

@@ -35,7 +35,7 @@

namespace geosx
{
using namespace dataRepository;
//using namespace dataRepository;
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
//using namespace dataRepository;

nice catch.

@klevzoff klevzoff merged commit 783b654 into develop Mar 5, 2021
@klevzoff klevzoff deleted the bugfix/klevzoff/vtk-output-dir branch March 5, 2021 03:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] VTK output does not respect output directory
3 participants