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

Feature t8 io #368

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from
Draft

Feature t8 io #368

wants to merge 14 commits into from

Conversation

Davknapp
Copy link
Collaborator

Implementation of a general IO Interface to centralize all IO-routines in t8code.
Goal of this Interface is to:

  • Move all IO routines (like gmsh-reader, vtk-output, netcdf-IO...) in one place
  • Implement general IO routines, especially for serial/parallel readers. For objects, that define a cmesh we can extend a serial reader (read one object on one master proc) to a parallel reader (read multiple objects, each defining a cmesh on multiple processes).
  • Avoid code duplication for similar IO-routines but for different objects.

This PR provides only the structure to meet these ideas. I will move the current implementations of supported IO-routines as soon as possible.

All these boxes must be checked by the reviewers before merging the pull request:

As a reviewer please read through all the code lines and make sure that the code is fully understood, bug free, well-documented and well-structured.

  • The author added a BSD statement to doc/ (or already has one)

  • The code compiles without warning in debugging and release mode, with and without MPI (this should be executed automatically in a github action)

    If the Pull request introduces code that is not covered by the github action (for example coupling with a new library):

    • Should this use case be added to the github action?
    • If not, does the specific use case compile and all tests pass (check manually)
  • All tests pass (in various configurations, this should be executed automatically in a github action)

  • New source/header files are properly added to the Makefiles

  • The reviewer executed the new code features at least once and checked the results manually

  • The code is covered in an existing or new test case

  • New tests use the Google Test framework

  • The code follows the t8code coding guidelines

  • The code is well documented

  • All function declarations, structs/classes and their members have a proper doxygen documentation

  • All new algorithms and data structures are sufficiently optimal in terms of memory and runtime (If this should be merged, but there is still potential for optimization, create a new issue)

  • Testing of this template: If you feel something is missing from this list, contact the developers

@jmark jmark self-requested a review November 22, 2022 14:36
@Davknapp Davknapp marked this pull request as draft November 22, 2022 14:37
@Davknapp Davknapp marked this pull request as ready for review November 22, 2022 16:22
@Davknapp Davknapp mentioned this pull request Nov 24, 2022
14 tasks
@Davknapp Davknapp added enhancement Enhances already existing code New feature Adds a new feature to the code labels Nov 24, 2022
Copy link
Collaborator

@jmark jmark left a comment

Choose a reason for hiding this comment

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

Really nice starting point for future discussion! :)


/**
* \file t8_IO.h
* Defines a class that combines input and outputroutines.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo.


/**
* A general routine for writing files in serial.
* TODO: provide implementation
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't like TODOs in PRs. You can remove this TODO since this PR is aimed at the API anyway. Specific implementations will follow in later PRs.


/**
* A general routine for writing files in parallel.
* TODO: provide implementation
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't like TODOs in PRs. You can remove this TODO since this PR is aimed at the API anyway. Specific implementations will follow in later PRs.

/* Write the output */
virtual t8_read_status_t read ();

virtual t8_read_status_t set_source (const t8_extern_t * source);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is a good idea and exactly what we wanted to avoid, right? We should talk about this. :)

* \param[in] source an object to be filled.
* \return t8_write_status_t T8_WRITE_FAIL if it wasn't able to set the source, T8_WRITE_SUCCESS otherwise
*/
virtual t8_read_status_t set_source (const t8_extern_t * source) = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This routine does not have be part of the base class. We should talk about this.


/* *INDENT-OFF* */
t8_write_status
t8_vtk_writer::set_dest (const t8_extern_t * dest)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The same problem as with set_source.

@Davknapp Davknapp marked this pull request as draft January 25, 2023 10:13
@jmark jmark added the draft Enhance the visibility that this is a draft. label Mar 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
draft Enhance the visibility that this is a draft. enhancement Enhances already existing code New feature Adds a new feature to the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants