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

Slight redesign of the I/O system and binary I/O for Array #90

Merged
merged 4 commits into from
Mar 31, 2022

Conversation

ardasener
Copy link
Contributor

The primary goal of this PR is to implement binary readers and writers for the newly added Array format. However the current class structure of the I/O system caused some issues with this. So this PR also features a slight redesign of the system to accommodate Array and make the library more future proof.

Previous Design

template <typename IDType, typename NNZType, typename ValueType>
class BinaryWriter : public Writer<IDType, NNZType, ValueType>,
                     public WritesCOO<IDType, NNZType, ValueType>,
                     public WritesCSR<IDType, NNZType, ValueType> {
public:
  BinaryWriter(std::string filename);
  ~BinaryWriter() = default;
  void WriteCOO(format::COO<IDType, NNZType, ValueType> *coo) const;
  void WriteCSR(format::CSR<IDType, NNZType, ValueType> *csr) const;

private:
  std::string filename_;
};

The main problem with this design is that to write an Array object, the user still has to pass three template types only one of which would be used. Similar situations would occur as new formats with more or less templates get added to the library.

New Design

class Writer {
public:
  virtual ~Writer() = 0;
};


template <typename IDType, typename NNZType, typename ValueType>
class BinaryWriterOrderTwo : public Writer
                     public WritesCOO<IDType, NNZType, ValueType>,
                     public WritesCSR<IDType, NNZType, ValueType> {
public:
  explicit BinaryWriterOrderTwo(std::string filename);
  ~BinaryWriterOrderTwo() = default;
  void WriteCOO(format::COO<IDType, NNZType, ValueType> *coo) const;
  void WriteCSR(format::CSR<IDType, NNZType, ValueType> *csr) const;

private:
  std::string filename_;
};


template <typename T>
class BinaryWriterOrderOne : public Writer
                     public WritesArray<T> {
public:
  explicit BinaryWriterOrderOne(std::string filename);
  ~BinaryWriterOrderOne() = default;
  void WriteArray(format::Array<T> *arr) const;

private:
  std::string filename_;
};

The new design is inspired heavily by the recently updated conversion system. However in this case CRTP is not necessary so it is omitted. For more information, please refer to #85.

@ardasener ardasener added state: review needed type: feature Brand new functionality, features, workflows, endpoints, etc priority: now Critical priority priority: soon High priority and removed priority: now Critical priority labels Mar 31, 2022
public:
virtual format::COO<VertexID, NumEdges, Weight> *ReadCOO() const = 0;
virtual format::Array<T> *ReadCOO() const = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be ReadArray, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, totally

};

template <class VertexID, typename NumEdges, typename Weight>
template <typename IDType, typename NNZType, typename ValueType>
class ReadsSparseFormat {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this whole interface is pointless at this point 🤔 what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should rename it as ReadOrderTwoFormat since that's what it's supposed to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to CLion, it is not used anywhere in the library. So I think we should just remove it. Graphs right now have methods for both CSR and COO so a generic one is not really necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can always add it back in if we end up needing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

fair enough.

Copy link
Contributor

@AmroAlJundi AmroAlJundi left a comment

Choose a reason for hiding this comment

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

Looks good. There's a typo + I think ReadsSparseFormat was meant for reading connectivity in graphs so maybe it should be ReadsOrderTwoFormat. What do you think?

@ardasener ardasener merged commit f3d312c into develop Mar 31, 2022
@ardasener ardasener deleted the feature/io_redesign branch March 31, 2022 14:24
@ardasener ardasener added state: approved Approved to proceed. Ready to be merged and removed state: review needed labels Mar 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: soon High priority state: approved Approved to proceed. Ready to be merged type: feature Brand new functionality, features, workflows, endpoints, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants