Skip to content

Utilize FourCIPP in MeshPy #354

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

davidrudlstorfer
Copy link
Collaborator

@davidrudlstorfer davidrudlstorfer commented May 19, 2025

Utilize FourCIPP in MeshPy (the input file inherits from it and also the testing uses a ready-to-use function from FourCIPP)

- The 4C input file now directly inherits from the FourCIPP input file
- The dict and list comparison now also utilizes utilities from FourCIPP
@davidrudlstorfer
Copy link
Collaborator Author

(Only the second commit is crucial - the first commit is from #348)

@davidrudlstorfer
Copy link
Collaborator Author

Sorry for the chaos - got it working locally - I am missing something somewhere

@davidrudlstorfer davidrudlstorfer force-pushed the use_fourcipp_type_converter branch from e7afc73 to 1b8dcc7 Compare May 19, 2025 13:03
@davidrudlstorfer
Copy link
Collaborator Author

This was one really dumb typo on the fourcipp side ... Now everything works as expected. No more type casting needed for any numpy type and our functions can also be automatically converted everywhere.

@davidrudlstorfer davidrudlstorfer mentioned this pull request May 19, 2025
@davidrudlstorfer davidrudlstorfer force-pushed the use_fourcipp_type_converter branch 2 times, most recently from e9d49b9 to de33f08 Compare May 20, 2025 11:09
@davidrudlstorfer davidrudlstorfer force-pushed the use_fourcipp_type_converter branch from de33f08 to b4cb809 Compare May 20, 2025 14:44
@davidrudlstorfer
Copy link
Collaborator Author

@isteinbrecher now this would also be ready from a FourCIPP converter perspective. Now nothing should change anymore - therefore, now it's completely ready for review:)

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 changes @davidrudlstorfer ! This was actually very easy to review considering the complexity.

I really like how using the FourCIPP data structures simplifies a lot of the implementation in MeshPy BeamMe.

Some of my raised points can also be tackles in follow up PRs. We can shortly discuss this in the threads and open issues if we think this is the better way to go.

I am getting very used to the new interface!

return len(dictionary[section_name])
else:
return 0
return len(self.sections.get("NODE COORDS", []))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Related to my previous comment, one general point (for this whole PR, especially this file):
If I understand this directly, FourCIPP will merge the normal sections with the legacy sections when interacting with self.section. Can we somehow just access the existing data without having to copy around everything? Maybe via self._sections or self._legacy_sections?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@gilrrei could you quickly help out here - thanks!

Copy link

Choose a reason for hiding this comment

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

@isteinbrecher @davidrudlstorfer, yes, for now, unfortunately, stuff needs to be copied. As mentioned, correctly pointed out by @isteinbrecher, you can access the _sections and _legacy_sections directly. Once this is changed in 4C, this can be improved. If a get function is method is desired in fourcipp (which can return a default value), feel free to implement it, it might have other use cases.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the explanation @gilrrei , just to be sure, I understand this correctly, we don't copy the entire contents of the dictionaries, basically only the keys and the pointers to the data, correct?

@davidrudlstorfer If that is the case, I think we can simply leave this as is right now, and maybe clean this up either when reworking the MeshPy output functionality or if the behaviour in fourcipp changes at some point.

@davidrudlstorfer
Copy link
Collaborator Author

@isteinbrecher this would be ready for your review again.

I left four comments open for discussion and fixed the one thing. The remaining points should probably be tackled in separate PR's as they are already documented in the corresponding issue (testing tolerances, ...)

@davidrudlstorfer
Copy link
Collaborator Author

With 4C-multiphysics/fourcipp#50 being merged I've switched back FourCIPP to the main repo again:)

Therefore, this would be finally ready:)

@davidrudlstorfer davidrudlstorfer marked this pull request as ready for review May 21, 2025 08:46
@davidrudlstorfer
Copy link
Collaborator Author

davidrudlstorfer commented May 21, 2025

I've copied the ToDo's from #348 (comment)






  • nurbs: fix dump_to_list functions to be compatible with fourcipp

  • fourcipp: Once the nurbs stuff is fixed we must remove the function is_fourcipp_available

  • fourcipp: a dumper for numpy types (arrays, floats, ints, booleans) will be added soon. Therefore, I would keep the int/float/list comparison for now in the dump_to_list functions and remove it soon from MeshPy. See Add converter to convert data to native Python data types 4C-multiphysics/fourcipp#50 and Utilize FourCIPP in MeshPy #354

  • currently the overwrite option is gone, this is not as easily implementable because one does actually not know what should be overwritten? If there is a plausible solution to this we can add it to FourCIPP we discussed that this feature is not necessary

  • can we finally delete the tutorial or should be wait longer?
 Create an issue for that Already documented in Rethink tutorials for MeshPy #171

After these things are fixed I would merge this and I noted the following clean-ups after this PR

  • move all dump_to_list logic to the input file (similar to boundary_condition, function, etc…)

  • move element_type_to_four_c_string to a more central place, i.e., out of core. This should be easily possible once the dump_to_list functions are in the input file (currently a circular import restricts me from moving this)

  • then we can also fix the current hackery in the solidrigidsphere for free (here also a circular import hinders me from working further on this)

  • discuss if the validation should be turned on during dump (note: this could lead to failing tests, i.e., some form of input changes in 4C - we pull the latest FourCIPP version nightly - then the validation would fail, note that this could also be a good thing)

  • Replace self.element_description with self.data.

  • Pass the connectivity array of elements into the input file and hand over the conversion for a MeshPy Node to the FourCIPP converter

  • Finally we can improve and simplify (by a lot) the model importer

  • And finally, finally we can improve and simplify the mesh to dict conversion (by a lot)

@davidrudlstorfer davidrudlstorfer changed the title Use fourcipp type converter Utilize FourCIPP in MeshPy May 21, 2025
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 again @davidrudlstorfer for the changes, and sorry for my overdue review. I aded one more point to your comment #354 (comment).

I think if we only have two minor points left, I am confident that we can merge this soon!

@@ -314,10 +312,27 @@ def _assert_results_equal(
# Dictionary comparison
if isinstance(reference, dict) or isinstance(result, dict):

def sanitize(obj):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think one of my comments of my original review somehow did not make it.

Can we, instead of defining our own converting function here, use the custom_compare argument to compare_nested_dicts_or_lists?

@isteinbrecher
Copy link
Collaborator

Oh, and when this is finished, should we simply copy the open points from #354 (comment) into a "cleanup issue"?

Comment on lines -690 to +640
set_binning_strategy_section(
input_file, option_overwrite=option_overwrite, **binning_parameters
)
set_binning_strategy_section(input_file, **binning_parameters)
Copy link
Collaborator

Choose a reason for hiding this comment

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

So is the default behavior now, that the parameter will be overwritten once header function is called?
@isteinbrecher are you happy with that? I just remember that for some testcases we would like to have the possibility not to overwrite some already set section parameters...

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