Skip to content

Remove inheritance of InputFile on Mesh #321

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

Conversation

davidrudlstorfer
Copy link
Collaborator

I decided to add one additional step before moving the input file to fourcipp - namely the removal of the inheritance of the InputFile on Mesh

The overall code changes are pretty minimal - but major changes must occur to the testing framework.

Following this PR - the change to fourcipp should be really straight forward

@davidrudlstorfer davidrudlstorfer self-assigned this Apr 11, 2025
@davidrudlstorfer davidrudlstorfer force-pushed the remove_inheritance_of_inputfile_on_mesh branch 2 times, most recently from 327ddeb to bbd1d58 Compare April 30, 2025 16:26
@davidrudlstorfer davidrudlstorfer force-pushed the remove_inheritance_of_inputfile_on_mesh branch from bbd1d58 to 8186f69 Compare April 30, 2025 16:37
@davidrudlstorfer davidrudlstorfer marked this pull request as ready for review April 30, 2025 16:42
@davidrudlstorfer
Copy link
Collaborator Author

@isteinbrecher as discussed today - this would be ready for review 😅

The changes to the testing framework were a lot of manual labor - which will be touched again with the follow up PR's. Therefore, I am still torn if it is the better approach to split everything up 🫣

This should at least be easier to review and I am curious about your opinion:)

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR removes the inheritance of InputFile from the Mesh class and updates the tests and internal APIs accordingly. Key changes include replacing direct InputFile instantiations with Mesh() in tests, updating calls to add objects (e.g. functions, boundary conditions) via the mesh attribute, and modifying internal methods in InputFile to delegate mesh operations to a composed Mesh object.

Reviewed Changes

Copilot reviewed 10 out of 12 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/test_performance.py Replaced "InputFile()" with "Mesh()" for creating performance test meshes
tests/test_nurbs.py Changed InputFile usage to Mesh for NURBS tests and updated add() calls
tests/test_meshpy.py Systematic migration from InputFile to Mesh across multiple mesh tests
tests/test_mesh_creation_functions.py Updated mesh creation functions to use Mesh instantiation consistently
tests/test_four_c_simulation.py Modified simulation tests to use Mesh and add it to the InputFile
tests/test_four_c.py Adjusted tests for 4C simulation by delegating mesh operations to Mesh
src/meshpy/four_c/input_file.py Updated InputFile to use composition (mesh attribute) instead of inheritance
tests/conftest.py Extended type hints in comparison functions for Mesh objects
Files not reviewed (2)
  • tests/reference-files/test_meshpy_comments_in_solid.4C.yaml: Language not supported
  • tests/reference-files/test_meshpy_comments_in_solid_initial.4C.yaml: Language not supported
Comments suppressed due to low confidence (2)

tests/test_performance.py:123

  • Verify that replacing 'InputFile()' with 'Mesh()' here aligns with downstream test expectations, and update any related documentation to clarify the new test setup.
mesh = Mesh()

tests/test_meshpy.py:179

  • Ensure that the migration from 'InputFile()' to 'Mesh()' is applied consistently, and that all function calls and node references correctly use the new 'mesh' attribute.
mesh = Mesh()

@davidrudlstorfer
Copy link
Collaborator Author

And I almost forgot these points:

  • This is a breaking change ⚠️ now input file does not inherit the mesh anymore
  • And I've also deleted one test because this should be useless with yaml?

@davidrudlstorfer
Copy link
Collaborator Author

And my final comment 😅

In the future we could provide a pytest fixture to create a mesh, this way we could remove all of the

# Create mesh
mesh = Mesh()

and just use

test_something_in_meshpy(mesh):

Copy link
Collaborator

@isteinbrecher isteinbrecher left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the effort @davidrudlstorfer and the clean implementation (also adapting the comments in the testing code!). I only have some minor comments.

I am also not sure about the best way to do big changes like this, but for now I appreciate this way, as it makes the review a lot easier. Also, I think only a small portion of tests will have to be changed again later (basically the ones that import an existing input file) as most of the changed test are already aligning with the envisioned future API.

@davidrudlstorfer
Copy link
Collaborator Author

@isteinbrecher this would be ready for review again - I hope I've incorporated all of you suggestions accordingly. Thanks for the review.

The main problem with many tests is that we import some existing files with a "full import" or not. Then we can no longer simply compare the mesh or the input file because in the end we have to check both. This should be refactored once the mesh moves out of the input file.

Copy link
Collaborator

@isteinbrecher isteinbrecher left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the effort @davidrudlstorfer , I only have one small question, but that does not hinder this PR.

The main problem with many tests is that we import some existing files with a "full import" or not. Then we can no longer simply compare the mesh or the input file because in the end we have to check both. This should be refactored once the mesh moves out of the input file.

I agree. Maybe we can do the following: The function that reads in existing meshes (I more and more think it makes sense that this is a function and not a class method of InputFile) should always return two objects:

input_file, mesh = read_existing_yaml_or_whatever_suitable_name(full_import=True/False)

If full_import is True, then the mesh will be filled with the relevant data, if full_import is False, then the mesh will be empty. Then we could have the following logic in testing:

@pytest.mark.parametrize("full_import", [True, False])
def test_space_time_straight(full_import, assert_results_requal):
    input_file, mesh = read_existing_yaml_or_whatever_suitable_name(full_import= full_import)
    # Actually logic of the test -> here we add to mesh
    input_file.add(mesh)
    assert_results_requal(reference, input_file)

This should work right away for both input types.

@davidrudlstorfer
Copy link
Collaborator Author

Thanks a lot for the effort @davidrudlstorfer , I only have one small question, but that does not hinder this PR.

The main problem with many tests is that we import some existing files with a "full import" or not. Then we can no longer simply compare the mesh or the input file because in the end we have to check both. This should be refactored once the mesh moves out of the input file.

I agree. Maybe we can do the following: The function that reads in existing meshes (I more and more think it makes sense that this is a function and not a class method of InputFile) should always return two objects:

input_file, mesh = read_existing_yaml_or_whatever_suitable_name(full_import=True/False)

If full_import is True, then the mesh will be filled with the relevant data, if full_import is False, then the mesh will be empty. Then we could have the following logic in testing:

@pytest.mark.parametrize("full_import", [True, False])
def test_space_time_straight(full_import, assert_results_requal):
    input_file, mesh = read_existing_yaml_or_whatever_suitable_name(full_import= full_import)
    # Actually logic of the test -> here we add to mesh
    input_file.add(mesh)
    assert_results_requal(reference, input_file)

This should work right away for both input types.

Thanks for the nice summary - that's exactly what I have imagined for the testing and also the function to read an existing input file.

The following PR will now remove the mesh completely from the input file. Therefore, I am merging this

@davidrudlstorfer davidrudlstorfer merged commit fa31632 into beamme-py:main May 6, 2025
10 checks passed
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.

2 participants