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

thinking about the data source, background loading and data transport mess #975

Draft
wants to merge 34 commits into
base: master
Choose a base branch
from

Conversation

reinago
Copy link
Member

@reinago reinago commented Jan 21, 2022

open questions:

This is just about the structure and the interfaces. Do we want to put more restrictions? Or Less?
@github-actions
Copy link

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

@github-actions
Copy link

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

@github-actions
Copy link

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

@github-actions
Copy link

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

@straubar straubar self-assigned this Jan 25, 2022
@github-actions
Copy link

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

@github-actions
Copy link

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

// here we should actually grab the frames from some background thread that reads ahead and stuff?
FrameType f;
f.Read(IndexToIStream(idx));
return std::make_unique<FrameType>(f);
Copy link
Contributor

Choose a reason for hiding this comment

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

Use std::move(f) and demand that FrameType is moveable (C++ concepts?)

@github-actions
Copy link

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

@github-actions
Copy link

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

@github-actions
Copy link

github-actions bot commented Feb 2, 2022

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

@github-actions
Copy link

github-actions bot commented Feb 2, 2022

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

@github-actions
Copy link

github-actions bot commented Feb 2, 2022

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

@github-actions
Copy link

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

}

// data will be controlled by this instance, caller loses access!
void SetData(uint8_t* data, std::size_t count, ImageElementType channelType, SizeType width = 1, SizeType height = 1, SizeType depth = 1) {
Copy link
Member

Choose a reason for hiding this comment

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

That's a unique_ptr then

}

template <typename ResultType>
std::vector<ResultType> GetCopy() {
Copy link
Member

Choose a reason for hiding this comment

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

The switch-case part in the following functions can be in a separate method using a functor for copy/set/get functionality

default:
throw std::logic_error("ImageFrame::GetCopy: invalid elementType");
}
return std::move(out);
Copy link
Member

Choose a reason for hiding this comment

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

No move on return


template<typename Dest, typename Source>
void CopyInto(std::vector<Dest>& out) {
std::transform(ViewAs<Source>(), ViewAs<Source>() + NumElements(), std::back_inserter(out),
Copy link
Member

Choose a reason for hiding this comment

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

Method hides whether reserve or resize should be used for out

// typename ChannelType does not work, that is not a static decision.
// For some data sources you only know after actually opening the file what's in it.
// TODO: volume ist ein byte-vektor und wir benutzen span. fertig.
template<uint8_t Dimensions>
Copy link
Member

Choose a reason for hiding this comment

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

Do we need dimension as static information since we do not change available methods?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was wondering whether we should disable some of the methods depending on Dimensions. I am really not sure whether we should just treat dims dynamically too. A transfer function is a perfectly OK volume, strictly speaking. It's a bit thin, but not broken or something...

Copy link
Member

Choose a reason for hiding this comment

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

With the dimension discerning types, we have either three statically different call transporting 1D, 2D, or 3D volumes, or yet another dispatch. I would argue for dimension as template parameter, if we have a significant gain from it.

@github-actions
Copy link

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

@github-actions
Copy link

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

@github-actions
Copy link

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

@github-actions
Copy link

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

@github-actions
Copy link

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

@github-actions
Copy link

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

@github-actions
Copy link

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

@github-actions
Copy link

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

@github-actions
Copy link

github-actions bot commented Mar 3, 2022

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

@github-actions
Copy link

github-actions bot commented Mar 4, 2022

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

@github-actions
Copy link

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

@github-actions
Copy link

Style check found formatting issues! Comment /format, to automatically commit the suggested changes.

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.

4 participants