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

Extension of element removal #2115

Merged
merged 4 commits into from May 10, 2018

Conversation

Projects
None yet
3 participants
@rinkk
Copy link
Member

rinkk commented Apr 26, 2018

  • works with arbitrary scalar arrays now (currently limited to int + double)
  • allows inclusion and exclusion of value ranges
@TomFischer
Copy link
Member

TomFischer left a comment

Did not check the ui-file. The remaining parts look good.

@rinkk rinkk force-pushed the rinkk:ElementRemoval branch from 98224e6 to 3195581 Apr 27, 2018

{
MeshLib::Properties const& properties = mesh.getProperties();
std::vector<std::string> const& names = properties.getPropertyVectorNames();
for (auto name : names)

This comment has been minimized.

@endJunction

endJunction May 3, 2018

Member

a const ref would save some copies here: auto const& name ...

This comment has been minimized.

@rinkk

rinkk May 7, 2018

Author Member

✔️

if (this->insideButton->isChecked())
toggleScalarEdits(false);
else
toggleScalarEdits(true);

This comment has been minimized.

@endJunction

endJunction May 3, 2018

Member

shorter version, and IMO also more readable:

toggleScalarEdits(insideButton->isChecked());

This comment has been minimized.

@rinkk

rinkk May 7, 2018

Author Member

✔️

if (this->scalarArrayCheckBox->isChecked())
on_scalarArrayCheckBox_toggled(true);
if (this->boundingBoxCheckBox->isChecked())
on_boundingBoxCheckBox_toggled(true);

This comment has been minimized.

@endJunction

endJunction May 3, 2018

Member

same applies here:

on_boundingBoxCheckBox_toggled(boundingBoxCheckBox->isChecked());

This comment has been minimized.

@rinkk

rinkk May 7, 2018

Author Member

✔️

Q_UNUSED(idx);
MeshLib::Mesh const* const mesh =
_project.getMesh(meshNameComboBox->currentText().toStdString());
MeshLib::Properties const& properties = mesh->getProperties();

This comment has been minimized.

@endJunction

endJunction May 3, 2018

Member

The mesh pointer is not checked before access, could be nullptr.

The properties variable is needed after the if (vec_name == "") return;

This comment has been minimized.

@rinkk

rinkk May 7, 2018

Author Member

Added the nullptr-check (although if there was a problem the programme would've crashed long before this point), but I'm not sure what you mean with your second remark.

The ```if```` applies if the currently selected mesh has no scalar arrays. In this case, the combobox will be empty and therefore the returned string will be empty. No range values can be computed and all related widgets will remain disabled.

If the has does have scalar arrays, vec_name cannot be empty, thus range values are computed, widgets are enabled, etc.

Does that answer your comment?

This comment has been minimized.

@endJunction

endJunction May 7, 2018

Member

The second remark is only for the order of the lines; Should be

if (vec_name.empty()) return;
auto mesh = getMesh();
auto properties = getProperties(mesh);

The properties are not needed in case the vec_name is empty...

This comment has been minimized.

@rinkk

rinkk May 9, 2018

Author Member

I see. Fixed! ✔️

if (materialIDCheckBox->isChecked())
on_materialIDCheckBox_toggled(true);
std::string const vec_name(scalarArrayComboBox->currentText().toStdString());
if (vec_name == "")

This comment has been minimized.

@endJunction

endJunction May 3, 2018

Member
if (vec_name.empty())

This comment has been minimized.

@rinkk

rinkk May 7, 2018

Author Member

✔️

this->outsideScalarMinEdit->setText(QString::number(*min));
this->outsideScalarMaxEdit->setText(QString::number(*max));
this->insideScalarMinEdit->setText(QString::number(*min));
this->insideScalarMaxEdit->setText(QString::number(*max));

This comment has been minimized.

@endJunction

endJunction May 3, 2018

Member

The int and double versions are equal up to the type. A lambda would remove the code duplication easily.

template <typename T> auto do_something = [&]() {
   MeshLib::PropertyVector<T> ...
}
searcher.searchByPropertyValue(property_value, property_name);
for (auto const& property_value : property_values)
{
std::size_t n_marked_elements = searcher.searchByPropertyValue<double>(

This comment has been minimized.

@endJunction

This comment has been minimized.

@rinkk

rinkk May 7, 2018

Author Member

I added a line below where the variable might change, so it cannot be const.

std::size_t n_marked_elements = searcher.searchByPropertyValue<double>(
property_name, property_value);
if (n_marked_elements == 0)
std::size_t n_marked_elements = searcher.searchByPropertyValue<int>(

This comment has been minimized.

@endJunction

endJunction May 3, 2018

Member

I've difficulties understanding the algorithm.
If we change the order of double and int, would the output be still the same?
How to add another type, e.g. float?

Same questions to the following searchPropertyRange() function.

This comment has been minimized.

@rinkk

rinkk May 7, 2018

Author Member

searchByPropertyValue just calls searchByPropertyValueRange. I've just put this here for backwards compatibility because searching for a specific value was used in a few tests IIRC.

Additional types would need to be implemented her by adding a line with the respective type. The search-method itself just checks if the value for each element is smaller/larger than the range bounds. We just need to test the type because the property vector won't be returned without knowing the correct type.

The order of type-tests shouldn't matter because only one of them can ever return a nonzero value. All the wrong types aren't tested at all because they return a nullptr (or rather a boost optional, but it's essentially the same result) and thus will automatically return zero.


bool outside = false;
if (outside_property_arg.isSet())
outside = true;

This comment has been minimized.

@endJunction

endJunction May 3, 2018

Member
bool const outside = outside_property_arg.isSet();

This comment has been minimized.

@rinkk

rinkk May 7, 2018

Author Member

✔️

property_name, property_value, property_value, false);
}

/// @tparam PROPERTY_TYPE integral type of the property

This comment has been minimized.

@endJunction

endJunction May 3, 2018

Member

I'm not sure I understand what you write here. What do you mean by "integral type"? I'm asking because in the above implementation you added double and int, where the former is not an "integral type".

This comment has been minimized.

@rinkk

rinkk May 7, 2018

Author Member

I have no idea, actually. I just copied the comment from the previous method, because the range-search basically replaces the (single-)value search. I have no idea who wrote that comment originally.

I've removed the word "integral", is that okay with you?

This comment has been minimized.

@endJunction
/// @param max_property_value maximum value of the given property for the
/// element not to be marked
/// @param outside_of if true, all values outside of the given range or
/// markedc, if false, all values inside the given range are marked

This comment has been minimized.

@endJunction

endJunction May 3, 2018

Member

typo in the first "marked"

This comment has been minimized.

@rinkk

rinkk May 7, 2018

Author Member

`✔️

/// element not to be marked
/// @param max_property_value maximum value of the given property for the
/// element not to be marked
/// @param outside_of if true, all values outside of the given range or

This comment has been minimized.

@endJunction

endJunction May 3, 2018

Member

the default value is not documented. I'd even recommend to remove the default value and always explicitly write the intentions.

This comment has been minimized.

@rinkk

rinkk May 9, 2018

Author Member

Removed the default value.

matchedIDs.push_back(i);
for (std::size_t i(0); i < pv->getNumberOfTuples(); ++i)
{
if ((*pv)[i] < min_property_value || (*pv)[i] > max_property_value)

This comment has been minimized.

@endJunction

endJunction May 3, 2018

Member

I suspect, that the implementation will break down if there are multiple components in the tuple.

This comment has been minimized.

@rinkk

rinkk May 7, 2018

Author Member

I suppose so. I'll add a check concerning the tuple size...

}
else
{
for (std::size_t i(0); i < pv->getNumberOfTuples(); ++i)

This comment has been minimized.

@endJunction

endJunction May 3, 2018

Member

This is std::copy_if

This comment has been minimized.

@rinkk

rinkk May 7, 2018

Author Member

Could you please help me with the lambda here? I have no idea how I would return the index of the element after checking if the range fits.

This comment has been minimized.

@endJunction

endJunction May 7, 2018

Member

Ah, the index ;)
Sorry, then the copy_if is not applicable. It looked so similar to push_back(*pv[i])...

@endJunction
Copy link
Member

endJunction left a comment

When the tuple size check is included,

@rinkk rinkk force-pushed the rinkk:ElementRemoval branch from 5d33280 to 8804a74 May 9, 2018

@endJunction endJunction merged commit 8156b05 into ufz:master May 10, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details

@rinkk rinkk deleted the rinkk:ElementRemoval 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.