Skip to content

First version - #1

Merged
S-Dafarra merged 15 commits into
masterfrom
firstVersion
Sep 2, 2020
Merged

First version#1
S-Dafarra merged 15 commits into
masterfrom
firstVersion

Conversation

@S-Dafarra

@S-Dafarra S-Dafarra commented Aug 26, 2020

Copy link
Copy Markdown
Collaborator

It adds the skeleton, a basic interface to a generic variable, and the interface to a Vector.

What is missing:

  • CI
  • README
  • Documentation
  • Interface for other types of variables (multidimensional arrays, cell array, struct arrays)

I may start adding some documentation in the next few days. Any help is welcome.

@S-Dafarra

Copy link
Copy Markdown
Collaborator Author

Notice that Span has been copied from iDynTree. I preferred to include it rather than introducing a dependency for a single header file.

@S-Dafarra

S-Dafarra commented Aug 26, 2020

Copy link
Copy Markdown
Collaborator Author

Even if the PR looks huge, a lot of the code has been copied. The only files that should be reviewed are the CMakelists and the "code" files, except Span.h and its corresponding test which has been imported from iDynTree.

Comment thread CMakeLists.txt Outdated

if (other.isComplex())
{
std::cerr << "[matioCpp::Vector::fromOther] The input variable is complex, this is not." << std::endl;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is not what?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Complex 😁

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ahhh, this in the C++ sense, that was not super clear for me.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I have opened an issue for fixing this #4

@GiulioRomualdi GiulioRomualdi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@S-Dafarra, that's really cool! Just two curiosities:

  1. Does it work with matrices?
  2. Does it handle eigen vectors/matrices? As far as I understood the vectors can be converted in span but the matrices not

* @param imaginaryData A void pointer to the (flattened) imaginary data. Check the documentation of initializeVariable for understanding how to obtain/interpret this vector.
* @return true in case the variable was correctly initialized.
*/
bool initializeComplexVariable(const std::string& name, const VariableType& variableType, const ValueType& valueType, const std::vector<size_t>& dimensions, void *realData, void *imaginaryData);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

With complex do you mean variables containing std::complex numbers?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, matio supports the definition of complex numbers by returning separate arrays for the real and the imaginary part.

* @param data A void pointer to the (flattened) data.
* @return true in case the variable was correctly initialized.
*/
bool initializeVariable(const std::string &name, const VariableType &variableType, const ValueType &valueType, const std::vector<size_t> &dimensions, void *data);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just a curiosity, do you decide to handle the VariableType with a variable rather than a template because it simplifies the interface?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

With this method, I need to call stuff in the pimpl, so it cannot be templated. For the same reason, I have that ugly void* at the end. Nevertheless, this is also the way matvar_t stores data inside. Its type is not known at compile time.

template<typename T>
matioCpp::Vector<T>::Vector()
{
static_assert (!std::is_same<T, bool>::value, "Vector<bool> is not supported." );

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It is a pity that std::vector<bool> does not have the data() method

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I have opened an issue for this #6

@GiulioRomualdi

Copy link
Copy Markdown

@S-Dafarra, that's really cool! Just two curiosities:

  1. Does it work with matrices?
  2. Does it handle eigen vectors/matrices? As far as I understood the vectors can be converted in span but the matrices not

Probably multidimensionalArray branch is the answer to both questions

@S-Dafarra

Copy link
Copy Markdown
Collaborator Author

@S-Dafarra, that's really cool! Just two curiosities:

  1. Does it work with matrices?
  2. Does it handle eigen vectors/matrices? As far as I understood the vectors can be converted in span but the matrices not

Probably multidimensionalArray branch is the answer to both questions

Yes, I am working on its implementation now. I am not considering matrices, but generic multidimensional arrays (i.e. with the number of dimensions >= 2). In principle, also vectors could be considered as multidimensional arrays, but I wanted to leverage the use of span for this set of objects.

For what concerns conversions to Eigen, I am avoiding them since I want to keep C++14 for the time being. Nevertheless, it should be pretty easy to define maps from matio-cpp objects, to eigen, provided that the number of dimensions is <=2.

@GiulioRomualdi

Copy link
Copy Markdown

For multidimensional array, you may have a look at mdspan

Interesting link: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/p0009r10.html

@S-Dafarra

Copy link
Copy Markdown
Collaborator Author

For multidimensional array, you may have a look at mdspan

Interesting link: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/p0009r10.html

Thanks, I'll take a look, even though it seems still a little far compared to span, which is also still experimental 🤔

@S-Dafarra
S-Dafarra merged commit bc33ca3 into master Sep 2, 2020
@S-Dafarra
S-Dafarra deleted the firstVersion branch September 7, 2020 09:31
@prashanthr05

Copy link
Copy Markdown
Contributor

@S-Dafarra May I still do a review of this PR?

@S-Dafarra

Copy link
Copy Markdown
Collaborator Author

It has already been merged, but feel free to comment if something is not clear. I can try to address the comments in another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants