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

Support properties that are indexed per-node in more mesh types #720

Merged
merged 4 commits into from
Jun 16, 2023
Merged

Support properties that are indexed per-node in more mesh types #720

merged 4 commits into from
Jun 16, 2023

Conversation

jtai-eq
Copy link
Contributor

@jtai-eq jtai-eq commented Apr 25, 2023

Writing a property on an unstructured mesh as support is currently supported with "cells" as indexable element, but not "nodes". It appears that the only change needed to support "nodes" as indexable element for, e.g., a TetraMesh is in the code that verifies the shape of the input array. Our proposed code change is similar to how nodes were added as indexable elements to structured meshes in PR #214.

We would like to add a test to this PR that covers writing nodes-indexed properties on unstructured, and possibly properties on unstructured grids more generally. Where do you propose that we add these tests, in tests/unit_tests/unstructured or in tests/unit_tests/property?

Jussi Aittoniemi and others added 2 commits April 20, 2023 08:25
@codecov-commenter
Copy link

codecov-commenter commented May 9, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.10 🎉

Comparison is base (b448609) 80.81% compared to head (b8f043b) 80.92%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #720      +/-   ##
==========================================
+ Coverage   80.81%   80.92%   +0.10%     
==========================================
  Files         190      190              
  Lines       32277    32377     +100     
==========================================
+ Hits        26084    26200     +116     
+ Misses       6193     6177      -16     
Impacted Files Coverage Δ
resqpy/property/_collection_get_attributes.py 84.54% <100.00%> (-0.05%) ⬇️

... and 21 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@andy-beer
Copy link
Contributor

Hello, Apologies for not responding sooner – for some reason I didn't get a notification when this pull request was raised. The code change looks fine.

Regarding unit tests, my feeling is the tests/unit_tests/property directory would be preferable. I would suggest a new test module (the existing property one is already overloaded!). A new module could be specific to UnstructuredGrid properties, so perhaps test_unstructured_grid_properties.py – I leave it up to you though.

Thanks for the contribution.

@jtai-eq
Copy link
Contributor Author

jtai-eq commented May 19, 2023

Thanks for the feedback. I added a new unit test file in /property, as you suggested. For now, it only contains one test, which tests writing and re-reading float-valued properties on a TetraMesh support, with nodes or cells as indexable element. Ideally this file should contain more tests using different unstructured grids, but it is a start.

@jtai-eq jtai-eq marked this pull request as ready for review May 19, 2023 13:28
@andy-beer
Copy link
Contributor

andy-beer commented May 22, 2023

The new unit test module is failing on the format checks. We use yapf to auto-format (and check the formatting) for resqpy – with the yapf options specified in the repo configuration. I think you will need to make the required changes on the fork before the code can be merged.

If you have yapf installed then the following command issued from the top level directory of the forked resqpy should make the required format changes:
yapf -i tests/unit_tests/property/test_unstructured_grid_properties.py

@jtai-eq

@andy-beer andy-beer merged commit 67e4268 into bp:master Jun 16, 2023
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