diff --git a/tomviz/Pipeline.cxx b/tomviz/Pipeline.cxx index 21b5de211..d369cd4a6 100644 --- a/tomviz/Pipeline.cxx +++ b/tomviz/Pipeline.cxx @@ -7,7 +7,6 @@ #include "DataSource.h" #include "DockerExecutor.h" #include "DockerUtilities.h" -#include "EmdFormat.h" #include "ExternalPythonExecutor.h" #include "ModuleManager.h" #include "Operator.h" @@ -180,11 +179,13 @@ Pipeline::Future* Pipeline::execute(DataSource* dataSource) auto operators = dataSource->operators(); if (operators.size() < 1) { + emit finished(); return emptyFuture(); } Operator* firstModifiedOperator = operators.first(); if (!isModified(dataSource, &firstModifiedOperator)) { + emit finished(); return emptyFuture(); } @@ -269,6 +270,37 @@ Pipeline::Future* Pipeline::execute(DataSource* ds, Operator* start, auto pipelineFuture = new PipelineFutureInternal( this, branchFuture->operators(), branchFuture, operators.last() == end); + + // After the pipeline finishes, move the modules to the back and + // remove the old child data source. + // If the operator to be removed has a child, move those modules to + // the back. Otherwise, move the transformed data source modules. + DataSource* oldChild = nullptr; + if (m_lastOperatorChildRemoved) { + // This indicates that an operator was just removed. Use that. + oldChild = m_lastOperatorChildRemoved; + m_lastOperatorChildRemoved = nullptr; + } else { + // Get either the child data source of the last operator or the + // transformed data source (which could be the root data source). + auto lastOp = operators.last(); + oldChild = lastOp->childDataSource() ? lastOp->childDataSource() + : transformedDataSource(); + } + + connect(pipelineFuture, &Pipeline::Future::finished, oldChild, + [this, oldChild]() { + auto newChild = transformedDataSource(); + if (newChild != oldChild) { + // If the child is not the same, move the modules to the new one + ModuleManager::instance().moveModules(oldChild, newChild); + if (!oldChild->forkable()) { + // Remove the old child data source if it was not forkable + ModuleManager::instance().removeChildDataSource(oldChild); + } + } + }); + connect(pipelineFuture, &Pipeline::Future::finished, this, &Pipeline::finished); @@ -355,57 +387,68 @@ void Pipeline::branchFinished() if (operators.isEmpty()) { return; } - auto start = future->operators().first()->dataSource(); + auto start = operators.first()->dataSource(); auto newData = future->result(); - // We only add the transformed child data source if the last operator - // doesn't already have an explicit child data source i.e. - // hasChildDataSource is true. + + // Set the output data on the final operator's child data source auto lastOp = start->operators().last(); - if (!lastOp->hasChildDataSource()) { - DataSource* newChildDataSource = nullptr; - if (lastOp->childDataSource() == nullptr) { - newChildDataSource = new DataSource("Output"); - newChildDataSource->setPersistenceState( - tomviz::DataSource::PersistenceState::Transient); - newChildDataSource->setForkable(false); - newChildDataSource->setParent(this); - lastOp->setChildDataSource(newChildDataSource); - auto rootDataSource = dataSource(); - // connect signal to flow units and spacing to child data source. - connect(dataSource(), &DataSource::dataPropertiesChanged, - [rootDataSource, newChildDataSource]() { - // Only flow the properties if no user modifications have been - // made. - if (!newChildDataSource->unitsModified()) { - newChildDataSource->setUnits(rootDataSource->getUnits(), - false); - double spacing[3]; - rootDataSource->getSpacing(spacing); - newChildDataSource->setSpacing(spacing, false); - } - }); - } + QString label = "Output"; + + auto child = lastOp->childDataSource(); + if (child) { + // Remove the old child, and create a new one from the output data. + // We will always use the output data for the final output. + label = child->label(); + ModuleManager::instance().removeChildDataSource(child); + lastOp->setChildDataSource(nullptr); + } - // Update the type if necessary - DataSource::DataSourceType type = DataSource::hasTiltAngles(newData) - ? DataSource::TiltSeries - : DataSource::Volume; - lastOp->childDataSource()->setData(newData); - lastOp->childDataSource()->setType(type); - lastOp->childDataSource()->dataModified(); - - if (newChildDataSource != nullptr) { - emit lastOp->newChildDataSource(newChildDataSource); - // Move modules from root data source. - ModuleManager::instance().moveModules(dataSource(), newChildDataSource); + // Remove any children produced by the operators. We currently do not + // support intermediate data sources. + for (auto op : operators) { + if (op->childDataSource()) { + ModuleManager::instance().removeChildDataSource(op->childDataSource()); + op->setChildDataSource(nullptr); } } - else { - // If this is the only operator, make sure the modules are moved down. - if (start->operators().size() == 1) - ModuleManager::instance().moveModules(dataSource(), - lastOp->childDataSource()); - } + + // Create one + child = new DataSource(label); + child->setPersistenceState(tomviz::DataSource::PersistenceState::Transient); + lastOp->setChildDataSource(child); + + // TODO: the following should be set to this, once we get + // intermediate datasets working. + // child->setForkable(hasChildDataSource()); + child->setForkable(false); + + // TODO: when we get intermediate datasets working, this data source + // should have the same pipeline, with pauses in the pipeline at + // forkable data sources. + child->setParent(this); + + auto rootDataSource = dataSource(); + // connect signal to flow units and spacing to child data source. + connect(dataSource(), &DataSource::dataPropertiesChanged, child, + [rootDataSource, child]() { + // Only flow the properties if no user modifications have been + // made. + if (!child->unitsModified()) { + child->setUnits(rootDataSource->getUnits(), false); + double spacing[3]; + rootDataSource->getSpacing(spacing); + child->setSpacing(spacing, false); + } + }); + + DataSource::DataSourceType type = DataSource::hasTiltAngles(newData) + ? DataSource::TiltSeries + : DataSource::Volume; + child->setData(newData); + child->setType(type); + child->dataModified(); + + emit lastOp->newChildDataSource(child); } void Pipeline::pause() @@ -483,22 +526,18 @@ void Pipeline::addDataSource(DataSource* dataSource) &Operator::newChildDataSource), [this](DataSource* ds) { addDataSource(ds); }); - // We need to ensure we move add datasource to the end of the branch - auto operators = op->dataSource()->operators(); - if (operators.size() > 1) { - auto transformedDataSourceOp = - findTransformedDataSourceOperator(op->dataSource()); - if (transformedDataSourceOp != nullptr) { - auto transformedDataSource = transformedDataSourceOp->childDataSource(); - transformedDataSourceOp->setChildDataSource(nullptr); - op->setChildDataSource(transformedDataSource); - emit operatorAdded(op, transformedDataSource); - } else { - emit operatorAdded(op); - } - } else { - emit operatorAdded(op); - } + // This might also be the root datasource, which is okay + auto oldChild = transformedDataSource(op->dataSource()); + + // This is just to make the modules "move down" in the view before + // the operator is ran. + DataSource* outputDS = nullptr; + auto transformedDataSourceOp = + findTransformedDataSourceOperator(op->dataSource()); + if (transformedDataSourceOp) + outputDS = oldChild; + + emit operatorAdded(op, outputDS); }); // Wire up operatorRemoved. TODO We need to check the branch of the // pipeline we are currently executing. @@ -509,21 +548,16 @@ void Pipeline::addDataSource(DataSource* dataSource) if (!op->isNew()) { m_operatorsDeleted = true; } - if (op->childDataSource() != nullptr) { - auto transformedDataSource = op->childDataSource(); - auto operators = op->dataSource()->operators(); - // We have an operator to move it to. - if (!operators.isEmpty()) { - auto newOp = operators.last(); - op->setChildDataSource(nullptr); - newOp->setChildDataSource(transformedDataSource); - emit newOp->dataSourceMoved(transformedDataSource); - } - // Clean it up - else { - transformedDataSource->removeAllOperators(); - transformedDataSource->deleteLater(); - } + + if (op->dataSource()->operators().isEmpty() && op->childDataSource()) { + // The last operator was removed. Move all the modules to the root. + ModuleManager::instance().moveModules(op->childDataSource(), + op->dataSource()); + ModuleManager::instance().removeChildDataSource(op->childDataSource()); + } else if (op->childDataSource()) { + // Save this so we can move the modules from it and delete it + // later. + m_lastOperatorChildRemoved = op->childDataSource(); } // If pipeline is running see if we can safely remove the operator diff --git a/tomviz/Pipeline.h b/tomviz/Pipeline.h index dbe6d179d..a93e6be3c 100644 --- a/tomviz/Pipeline.h +++ b/tomviz/Pipeline.h @@ -143,6 +143,7 @@ private slots: QScopedPointer m_executor; ExecutionMode m_executionMode = Threaded; int m_editingOperators = 0; + DataSource* m_lastOperatorChildRemoved = nullptr; }; /// Return from getCopyOfImagePriorTo for caller to track async operation. diff --git a/tomviz/PipelineExecutor.cxx b/tomviz/PipelineExecutor.cxx index e19a2dad2..db75af702 100644 --- a/tomviz/PipelineExecutor.cxx +++ b/tomviz/PipelineExecutor.cxx @@ -83,7 +83,6 @@ Pipeline::Future* ExternalPipelineExecutor::execute(vtkDataObject* data, if (!m_temporaryDir->isValid()) { displayError("Directory Error", "Unable to create temporary directory."); return Pipeline::emptyFuture(); - ; } QString origFileName = originalFileName(); @@ -169,12 +168,10 @@ Pipeline::Future* ExternalPipelineExecutor::execute(vtkDataObject* data, &ExternalPipelineExecutor::pipelineStarted); connect(m_progressReader.data(), &ProgressReader::pipelineFinished, this, [this, future]() { + // Read the modified active scalars auto transformedFilePath = QDir(workingDir()).filePath(TRANSFORM_FILENAME); - vtkSmartPointer transformedData = - vtkImageData::New(); - vtkImageData* transformedImageData = - vtkImageData::SafeDownCast(transformedData.Get()); + vtkNew transformedImageData; // Make sure we don't ask the user about subsampling QVariantMap options = { { "askForSubsample", false } }; if (EmdFormat::read(transformedFilePath.toLatin1().data(), @@ -186,7 +183,6 @@ Pipeline::Future* ExternalPipelineExecutor::execute(vtkDataObject* data, .arg(transformedFilePath)); } emit future->finished(); - transformedImageData->FastDelete(); }); connect(future, &Pipeline::Future::finished, this, &ExternalPipelineExecutor::reset); @@ -250,11 +246,6 @@ void ExternalPipelineExecutor::operatorStarted(Operator* op) { op->setState(OperatorState::Running); emit op->transformingStarted(); - - auto pythonOp = qobject_cast(op); - if (pythonOp != nullptr) { - pythonOp->createChildDataSource(); - } } void ExternalPipelineExecutor::operatorFinished(Operator* op) @@ -277,7 +268,6 @@ void ExternalPipelineExecutor::operatorFinished(Operator* op) if (EmdFormat::read(fileInfo.filePath().toLatin1().data(), childData, options)) { childOutput[name] = childData; - emit pipeline()->finished(); } else { displayError( "Read Error", diff --git a/tomviz/PipelineModel.cxx b/tomviz/PipelineModel.cxx index c9db88e73..e84c84136 100644 --- a/tomviz/PipelineModel.cxx +++ b/tomviz/PipelineModel.cxx @@ -204,8 +204,6 @@ bool PipelineModel::TreeItem::remove(DataSource* source) // Resume but don't execute as we are removing this data source. pipeline->resume(); } - } else if (childItem->module()) { - ModuleManager::instance().removeModule(childItem->module()); } } if (parent()) { diff --git a/tomviz/Tvh5Format.cxx b/tomviz/Tvh5Format.cxx index 78131993f..f27d2238e 100644 --- a/tomviz/Tvh5Format.cxx +++ b/tomviz/Tvh5Format.cxx @@ -180,7 +180,6 @@ bool Tvh5Format::loadDataSource(h5::H5ReadWrite& reader, if (parent) { // This is a child data source. Hook it up to the operator parent. parent->setChildDataSource(dataSource); - parent->setHasChildDataSource(true); parent->newChildDataSource(dataSource); // If it has a parent, it will be deserialized later. } else { diff --git a/tomviz/operators/Operator.cxx b/tomviz/operators/Operator.cxx index d20d3af8c..7fad28d0d 100644 --- a/tomviz/operators/Operator.cxx +++ b/tomviz/operators/Operator.cxx @@ -193,23 +193,26 @@ void Operator::createNewChildDataSource( const QString& label, vtkSmartPointer childData, DataSource::DataSourceType type, DataSource::PersistenceState state) { - if (this->childDataSource() == nullptr) { - DataSource* childDS = - new DataSource(vtkImageData::SafeDownCast(childData), type, - this->dataSource()->pipeline(), state); - childDS->setLabel(label); - setChildDataSource(childDS); - setHasChildDataSource(true); - emit Operator::newChildDataSource(childDS); + auto child = childDataSource(); + if (!child) { + child = new DataSource(vtkImageData::SafeDownCast(childData), type, + this->dataSource()->pipeline(), state); + child->setLabel(label); + setChildDataSource(child); + emit Operator::newChildDataSource(child); } // Reuse the existing "Output" data source. else { - childDataSource()->setData(childData); - childDataSource()->setLabel(label); - childDataSource()->setForkable(true); - childDataSource()->dataModified(); - setHasChildDataSource(true); + child->setData(childData); + child->setType(type); + child->setLabel(label); + child->setPersistenceState(state); + child->dataModified(); } + // TODO: the following should be set to this, once we get + // intermediate datasets working. + // child->setForkable(hasChildDataSource()); + child->setForkable(false); } void Operator::cancelTransform() diff --git a/tomviz/operators/Operator.h b/tomviz/operators/Operator.h index c429ad959..7b33b3b0c 100644 --- a/tomviz/operators/Operator.h +++ b/tomviz/operators/Operator.h @@ -82,9 +82,19 @@ class Operator : public QObject virtual OperatorResult* resultAt(int index) const; /// Set whether the operator is expected to produce a child DataSource. + /// TODO: this is beginning to take on a new meaning, since every + /// operator now produces a child data source. The new meaning is: + /// should the child data source be kept and not thrown away when + /// another operator is performed on it? + /// Maybe we should change the naming? virtual void setHasChildDataSource(bool value); /// Get whether the operator is expected to produce a child DataSource. + /// TODO: this is beginning to take on a new meaning, since every + /// operator now produces a child data source. The new meaning is: + /// should the child data source be kept and not thrown away when + /// another operator is performed on it? + /// Maybe we should change the naming? virtual bool hasChildDataSource() const; /// Set the child DataSource. Can be nullptr. diff --git a/tomviz/operators/OperatorPython.cxx b/tomviz/operators/OperatorPython.cxx index cb1a4d63a..dc2cb893e 100644 --- a/tomviz/operators/OperatorPython.cxx +++ b/tomviz/operators/OperatorPython.cxx @@ -167,13 +167,6 @@ OperatorPython::OperatorPython(DataSource* parentObject) this, SLOT(updateChildDataSource(vtkSmartPointer)), connectionType); - // This connection is needed so we can create new child data sources in the UI - // thread from a pipeline worker threads. - connect(this, SIGNAL(newChildDataSource(const QString&, - vtkSmartPointer)), - this, SLOT(createNewChildDataSource(const QString&, - vtkSmartPointer)), - connectionType); connect( this, SIGNAL(newOperatorResult(const QString&, vtkSmartPointer)), @@ -264,22 +257,26 @@ void OperatorPython::setJSONDescription(const QString& str) QJsonObject::size_type size = childDatasetArray.size(); if (size != 1) { qCritical() << "Only one child dataset is supported for now. Found" - << size << " but only the first will be used"; + << size << "but only one will be used"; } if (size > 0) { - setHasChildDataSource(true); QJsonObject dataSourceNode = childDatasetArray[0].toObject(); QJsonValueRef nameValue = dataSourceNode["name"]; QJsonValueRef labelValue = dataSourceNode["label"]; - if (!nameValue.isUndefined() && !nameValue.isNull() && - !labelValue.isUndefined() && !labelValue.isNull()) { + if (!nameValue.isUndefined() && !nameValue.isNull()) { + // This variable is currently unused m_childDataSourceName = nameValue.toString(); + } + if (!labelValue.isUndefined() && !labelValue.isNull()) { m_childDataSourceLabel = labelValue.toString(); - } else if (nameValue.isNull()) { - qCritical() << "No name given for child DataSet"; } else if (labelValue.isNull()) { qCritical() << "No label given for child DataSet"; } + + // TODO: when we add intermediate data sources, we should add + // an option to the description.json to keep intermediate data + // sources, such as this option. + // setHasChildDataSource(dataSourceNode["keepIntermediate"].toBool()); } } @@ -347,36 +344,32 @@ void OperatorPython::setScript(const QString& str) void OperatorPython::createChildDataSource() { - // Create child datasets in advance. Keep a map from DataSource to name - // so that we can match Python script return dictionary values containing - // child data after the script finishes. - if (hasChildDataSource() && !childDataSource()) { - // Create uninitialized data set as a placeholder for the data - vtkSmartPointer childData = - vtkSmartPointer::New(); - childData->ShallowCopy( - vtkImageData::SafeDownCast(dataSource()->dataObject())); - emit newChildDataSource(m_childDataSourceLabel, childData); - } + if (childDataSource()) + return; + + vtkNew childData; + childData->ShallowCopy(dataSource()->imageData()); + createNewChildDataSource(m_childDataSourceLabel, childData); } bool OperatorPython::updateChildDataSource(Python::Dict outputDict) { - if (hasChildDataSource()) { - Python::Object pyDataObject; - pyDataObject = outputDict[m_childDataSourceName]; - if (!pyDataObject.isValid()) { - qCritical() << "No child dataset named " << m_childDataSourceName - << "defined in dictionary returned from Python script.\n"; - return false; - } else { - vtkObjectBase* vtkobject = Python::VTK::convertToDataObject(pyDataObject); - vtkDataObject* dataObject = vtkDataObject::SafeDownCast(vtkobject); - if (dataObject) { - TemporarilyReleaseGil releaseMe; - emit childDataSourceUpdated(dataObject); - } - } + QStringList keys = outputDict.keys(); + if (keys.isEmpty()) + return false; + + if (keys.size() > 1) { + qCritical() << "Warning: more than one key was received in " + << "updateChildDataSource. Only one will be used"; + } + + Python::Object pyDataObject = outputDict[keys[0]]; + + vtkObjectBase* vtkobject = Python::VTK::convertToDataObject(pyDataObject); + vtkDataObject* dataObject = vtkDataObject::SafeDownCast(vtkobject); + if (dataObject) { + TemporarilyReleaseGil releaseMe; + emit childDataSourceUpdated(dataObject); } return true; @@ -385,21 +378,18 @@ bool OperatorPython::updateChildDataSource(Python::Dict outputDict) bool OperatorPython::updateChildDataSource( QMap> output) { - if (hasChildDataSource()) { - for (QMap>::const_iterator iter = - output.begin(); - iter != output.end(); ++iter) { - - if (iter.key() != m_childDataSourceName) { - qCritical() << "No child dataset named " << m_childDataSourceName - << "defined in dictionary returned from Python script.\n"; - return false; - } + if (output.isEmpty()) { + qCritical() << "No output found in updateChildDataSource"; + return false; + } - emit childDataSourceUpdated(iter.value()); - } + if (output.size() > 1) { + qCritical() << "Warning: more than one output was received in " + << "updateChildDataSource. Only one will be used"; } + emit childDataSourceUpdated(output.first()); + return true; } @@ -414,8 +404,6 @@ bool OperatorPython::applyTransform(vtkDataObject* data) Q_ASSERT(data); - createChildDataSource(); - Python::Object result; { Python python; @@ -457,18 +445,22 @@ bool OperatorPython::applyTransform(vtkDataObject* data) check = result.isDict(); } + // Record whether an output was encountered (i. e., a dict was + // returned from the python function that contains a child dataset) + bool outputEncountered = false; + bool errorEncountered = false; if (check) { Python python; Python::Dict outputDict = result.toDict(); - // Support setting child data from the output dictionary - errorEncountered = !updateChildDataSource(outputDict); - // Results (tables, etc.) for (int i = 0; i < m_resultNames.size(); ++i) { - Python::Object pyDataObject; - pyDataObject = outputDict[m_resultNames[i]]; + Python::Object pyDataObject = outputDict[m_resultNames[i]]; + + // Remove the item from the dictionary so that we do not think + // it is a child after this loop. + outputDict.delItem(m_resultNames[i]); if (!pyDataObject.isValid()) { errorEncountered = true; @@ -489,13 +481,24 @@ bool OperatorPython::applyTransform(vtkDataObject* data) } } - if (errorEncountered) { + // At this point, all results should have been removed from the + // dictionary. Only children should remain. + if (!errorEncountered && outputDict.size() > 0) { + outputEncountered = true; + errorEncountered = !updateChildDataSource(outputDict); + } + if (errorEncountered) { qCritical() << "Dictionary return from Python script is:\n" << outputDict.toString(); } } + if (outputEncountered) { + // Set the output data on the vtkDataObject + data->ShallowCopy(childDataSource()->dataObject()); + } + return !errorEncountered; } @@ -646,8 +649,13 @@ EditOperatorWidget* OperatorPython::getEditorContentsWithData( void OperatorPython::updateChildDataSource(vtkSmartPointer data) { - // Check to see if a child data source has already been created. If not, - // create it here. + // This function can be called either when a progress update is + // performed, or when an output dict was returned from a python + // operation. Both are stored in the child data source. + + if (!childDataSource()) + createChildDataSource(); + auto dataSource = childDataSource(); Q_ASSERT(dataSource); diff --git a/tomviz/operators/ReconstructionOperator.cxx b/tomviz/operators/ReconstructionOperator.cxx index 46caeb0dd..80bf6967d 100644 --- a/tomviz/operators/ReconstructionOperator.cxx +++ b/tomviz/operators/ReconstructionOperator.cxx @@ -36,7 +36,6 @@ ReconstructionOperator::ReconstructionOperator(DataSource* source, QObject* p) } setSupportsCancel(true); setTotalProgressSteps(m_extent[1] - m_extent[0] + 1); - setHasChildDataSource(true); connect( this, static_castShallowCopy(reconstructionImage); return true; } } // namespace tomviz diff --git a/tomviz/python/tomviz/executor.py b/tomviz/python/tomviz/executor.py index 88f7447c1..a977cdc16 100644 --- a/tomviz/python/tomviz/executor.py +++ b/tomviz/python/tomviz/executor.py @@ -10,7 +10,6 @@ import abc import stat import json -import six import errno from tqdm import tqdm @@ -684,7 +683,7 @@ def _load_transform_functions(operators): def _write_child_data(result, operator_index, output_file_path, dims): - for (label, dataobject) in six.iteritems(result): + for (label, dataobject) in result.items(): # Only need write out data if the operator made updates. output_path = '.' if output_file_path is not None: @@ -758,6 +757,12 @@ def execute(operators, start_at, data_file_path, output_file_path, _write_child_data(result, operator_index, output_file_path, dims) + # Update the data with the result + if len(result) > 1: + logger.warning('Multiple results found. ' + 'Only one will be used') + data = next(iter(result.values())) + progress.finished(operator_index) operator_index += 1