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

Boundary conditions / source terms in DataExplorer #2110

Merged
merged 19 commits into from Apr 23, 2018

Conversation

Projects
None yet
3 participants
@rinkk
Copy link
Member

rinkk commented Apr 6, 2018

Current state is

  • limited reading / writing of prj files (i.e. it works in principle but some issues are currently ignored)
  • data structures for holding data for DE
  • listing information on process vars / bc's / st's in DE data view
@bilke

This comment has been minimized.

Copy link
Member

bilke commented Apr 9, 2018

Jenkins tests do not run because:

scripts/cmake/CompilerSetup.cmake:139: trailing whitespace.
/// Displays information on a process variable
void setProcessVariable(DataHolderLib::FemCondition* cond);

private:

This comment has been minimized.

@endJunction

endJunction Apr 9, 2018

Member

could be removed


#include "Applications/DataHolderLib/FemCondition.h"

// class vtkUnstructuredGridAlgorithm;

This comment has been minimized.

@endJunction

endJunction Apr 9, 2018

Member

please remove unused code snippets

This comment has been minimized.

@rinkk

rinkk Apr 10, 2018

Author Member

You most likely noticed that some of the files are updated versions of the files I removed a few years back when we removed BCs/STs because of the upcoming changes to OGS. I've kept these code snippets in because they're part of the next steps I mentioned in the OP (i.e. adding graphical representations + the option to create/modify BCs). If you insist I'll remove them for now but basically that's just adding work because I'd have to keep back-ups locally and would edit these in later anyway.

This comment has been minimized.

@endJunction

endJunction Apr 10, 2018

Member

No problem. I thought it would be debugging/developing leftovers.

// ThirdParty/logog
#include "logog/include/logog.hpp"

// ** INCLUDES **

This comment has been minimized.

@endJunction

endJunction Apr 9, 2018

Member

misplaced and actually unnecessary.

* \brief Implementation of the ProcessModel class.
*
* \copyright
* Copyright (c) 2013, OpenGeoSys Community (http://www.opengeosys.org)

This comment has been minimized.

@endJunction

endJunction Apr 9, 2018

Member

copyright years

#include "ProcessVarItem.h"

#include <vtkPolyDataAlgorithm.h>
#include <QFileInfo>

This comment has been minimized.

@endJunction

endJunction Apr 9, 2018

Member

it is custom to include the system headers after the "ProcessModel.h"
logog is also a system header...

_rootItem = new TreeItem(rootData, nullptr);
}

ProcessModel::~ProcessModel() {}

This comment has been minimized.

@endJunction

endJunction Apr 9, 2018

Member

better to write ~ProcessModel() = default; in the class definition.

conditions)
{
for (size_t i = 0; i < conditions.size(); i++)
addCondition(conditions[i].get());

This comment has been minimized.

@endJunction

endJunction Apr 9, 2018

Member

Please use the for-range-loop when the counting variable is not needed.

for (auto& c : conditions)
{
    addCondition(*c);
}

No need to use std::unique_ptr<T>::get(). A unique_ptr is a pointer too, so use dereference operator.

prefix size_t with the std namespace...

if (pv_item == nullptr)
return;

int n_conds = pv_item->childCount();

This comment has been minimized.

@endJunction

endJunction Apr 9, 2018

Member

n_conds is a constant....

static_cast<CondItem*>(pv_item->child(i))->getName());

_project.removePrimaryVariable(name.toStdString());
int idx = pv_item->row();

This comment has been minimized.

@endJunction

endJunction Apr 9, 2018

Member

also a constant....

signals:
};

#endif // PROCESSMODEL_H

This comment has been minimized.

@endJunction

endJunction Apr 9, 2018

Member

use pragma once, please.

{
}

~ProcessVarItem() {}

This comment has been minimized.

@endJunction

endJunction Apr 9, 2018

Member

= default would be better.

static_cast<ProcessModel*>(this->model())->getItem(idx));
if (item != nullptr)
return true;
return false;

This comment has been minimized.

@endJunction

endJunction Apr 9, 2018

Member

could be one-liner:

return dynamic_cast<ProcessVarItem*>(...) != nullptr;

If you like the temporary item variable, the if-condition is to be replaced with single return statement anyway.

* Terms) with a number of additional information such as Process Type,
* Distribution, etc. \sa ConditionModel, CondItem
*/
class ProcessView : public QTreeView

This comment has been minimized.

@endJunction

endJunction Apr 9, 2018

Member

classes, from which a user is not supposed to derive another class, should be declared with final specifier.

@@ -1,9 +1,4 @@
/**
* \file

This comment has been minimized.

@endJunction

endJunction Apr 9, 2018

Member

Please keep the \file line, otherwise the file comments in the doxygen will also belong to other files.

class BoundaryCondition : public DataHolderLib::FemCondition
{
public:
enum ConditionType

This comment has been minimized.

@endJunction

endJunction Apr 9, 2018

Member

I'm not sure, if you rely on implicit conversion of the enum to integer types, but if not, please make the enum an enum class. If you rely on the implicit conversions, consider to add explicit type conversions.

FemCondition(ProcessVariable const& process_var,
std::string const& param_name);

~FemCondition() {}

This comment has been minimized.

@endJunction

endJunction Apr 9, 2018

Member

dtor must be virtual.

virtual ~FemCondition() = default;
{
std::size_t n_bc(_boundary_conditions.size());
for (std::size_t i = 0; i < n_bc; ++i)
if (_boundary_conditions[i]->getProcessVarName() == primary_var_name)

This comment has been minimized.

@endJunction

endJunction Apr 9, 2018

Member

since i is used only to access the boundary condition, a for-range-loop is more appropriate.

void Project::removeBoundaryCondition(std::string const primary_var_name,
std::string const& param_name)
{
std::size_t n_bc(_boundary_conditions.size());

This comment has been minimized.

@endJunction
}

void Project::removeBoundaryCondition(std::string const primary_var_name,
std::string const& param_name)

This comment has been minimized.

@endJunction

endJunction Apr 9, 2018

Member

probably missing a reference for the first argument.

_boundary_conditions[i]->getParamName() == param_name)
{
BoundaryCondition* bc = _boundary_conditions[i].release();
delete bc;

This comment has been minimized.

@endJunction

endJunction Apr 9, 2018

Member

I think that

_boundary_conditions[i].reset();

is more idiomatic than having to delete a raw-pointer.

SourceTerm(ProcessVariable const& process_var,
std::string const& param_name, ConditionType type);

~SourceTerm() {}

This comment has been minimized.

@endJunction

endJunction Apr 9, 2018

Member

I would think the default implementation is not needed at all...

ERR("reading conditions");
}
else
ERR("not found");

This comment has been minimized.

@endJunction

endJunction Apr 9, 2018

Member

please, be consistent with the braces.

geo_objects.getGeometryNames(geo_names);
for (std::vector<std::string>::const_iterator it(geo_names.begin());
it != geo_names.end();
++it)

This comment has been minimized.

@endJunction

endJunction Apr 9, 2018

Member

can we get rid of those iterators, please! This is so c++03 style.
There are for-range-loops, which are readable.

std::vector<DataHolderLib::ProcessVariable> const& p_vars)
{
std::size_t const n_vars(p_vars.size());
for (DataHolderLib::ProcessVariable const p_var : p_vars)

This comment has been minimized.

@endJunction

endJunction Apr 9, 2018

Member

this is already implemented in the STL:

return std::any_of(p_vars.begin(), p_vars.end(),
    [&](auto const& p_var){ return p_var.name == name;});
XMLQtInterface(
BaseLib::FileFinder({BaseLib::BuildInfo::app_xml_schema_path})
.getPath("OpenGeoSysProject.xsd")),
_project(project)

This comment has been minimized.

@endJunction

endJunction Apr 9, 2018

Member

_filename is not initialized in the ctor. Is it correct?

@@ -135,7 +135,8 @@ if(WIN32)
/wd4267 /wd4996 /bigobj")
set(CMAKE_CXX_FLAGS_RELWITHDEBINFO "${CMAKE_CXX_FLAGS_RELWITHDEBINFO} \
/ZI /Od /Ob0")

set(CMAKE_EXE_LINKER_FLAGS_DEBUG "${CMAKE_EXE_LINKER_FLAGS_DEBUG} /ignore:4099")

This comment has been minimized.

@endJunction

endJunction Apr 9, 2018

Member

I know, that other magic numbers used in disabling of warnings etc. are not explained, but it would be a good starting point to add a short comment about 4099

{
// write station file
GeoLib::IO::XmlStnInterface stn(geo_objects);
std::string name(*it);
std::string name(name);

This comment has been minimized.

@endJunction

endJunction Apr 10, 2018

Member

I'd guess this line is not needed. If the non-const version of name is needed, remove the const in the for-loop.

@rinkk rinkk force-pushed the rinkk:DataExplorerConditions branch 2 times, most recently from 614360d to 1b45616 Apr 18, 2018

@endJunction endJunction force-pushed the rinkk:DataExplorerConditions branch 2 times, most recently from 373ef34 to 987c7f8 Apr 22, 2018

@endJunction endJunction force-pushed the rinkk:DataExplorerConditions branch from 987c7f8 to 32028da Apr 22, 2018

@endJunction endJunction merged commit 4cdbf07 into ufz:master Apr 23, 2018

0 of 2 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details

@rinkk rinkk deleted the rinkk:DataExplorerConditions branch Oct 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.