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

Issue 108 #157

Open
wants to merge 31 commits into
base: devel
Choose a base branch
from
Open

Issue 108 #157

wants to merge 31 commits into from

Conversation

lsawade
Copy link
Collaborator

@lsawade lsawade commented Nov 22, 2024

Description

This Pull request is addressing #108 and sub issues:

Checklist

Please make sure to check developer documentation on specfem docs.

  • I ran the code through pre-commit to check style
  • My code passes all the integration tests
  • I have added sufficient unittests to test my changes
  • I have added/updated documentation for the changes I am proposing
  • I have updated CMakeLists to ensure my code builds
  • My code builds across all platforms

@buildbot-princeton
Copy link
Collaborator

Can one of the admins verify this patch?

2 similar comments
@buildbot-princeton
Copy link
Collaborator

Can one of the admins verify this patch?

@buildbot-princeton
Copy link
Collaborator

Can one of the admins verify this patch?

@lsawade lsawade changed the base branch from main to devel November 22, 2024 21:08

namespace specfem {
namespace IO {
namespace mesh {
Copy link
Collaborator

@Rohit-Kakodkar Rohit-Kakodkar Nov 23, 2024

Choose a reason for hiding this comment

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

Can we move these functions within the impl namespace i.e. specfem::IO::impl::fortran::read_boundaries. Functions within impl namespace are implementation details and are not meant to exposed to the developer or directly used by another module within SPECFEM++. I started following this convension recently so only the recent implementations are compliant with this philosophy.

template specfem::mesh::interface_container<
specfem::element::medium_tag::elastic,
specfem::element::medium_tag::acoustic>
specfem::IO::mesh::fortran::read_interfaces<
Copy link
Collaborator

Choose a reason for hiding this comment

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

You dont nacessarily need this explicit instantiation here, since, this function is only ever used by read_coupled_interfaces. read_coupled_interfaces knows the definition from above function and wouldn't use the explicit instantiation I believe.

@@ -1,6 +1,8 @@
#include "compute/interface.hpp"
// #include "coupled_interface/interface.hpp"
// #include "domain/interface.hpp"
#include "IO/mesh/read_mesh.hpp"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to have both includes within the same header? What would be the best name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking about this while I was creating the read_sources. I think:

IO should contain read_mesh, read_sources, and read_receivers.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you mean we should move other IO routines fortran, HDF5 and ASCII to another namespace?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will create IO/interface.hpp for the implementations of the read_<xxx> functions, such that read_sources, read_mesh, and read_receivers are in the IO namespace under the interface header.

Copy link
Collaborator

@Rohit-Kakodkar Rohit-Kakodkar left a comment

Choose a reason for hiding this comment

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

Do you mean we should move other IO routines fortran, HDF5 and ASCII to another namespace?

@Rohit-Kakodkar
Copy link
Collaborator

retest this please

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.

3 participants