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

Add to_xarray as field method #123

Merged
merged 61 commits into from Mar 23, 2022
Merged

Add to_xarray as field method #123

merged 61 commits into from Mar 23, 2022

Conversation

swapneelap
Copy link
Member

@swapneelap swapneelap commented Feb 2, 2022

Adding to_xarray method for Field object which returns field value as an xarray.DataArray.

  1. Typically, the returned DataArray has four dimensions, namely x, y, z, and comp. The first three corresponds to geometry while the fourth dimension corresponds to the components of field. comp dimension has field.components as co-ordinates.
  2. If the Field object is a scalar field, the comp dimension is 'squeezed' and the DataArray has only three dimensions corresponding to the geometry.
  3. Instead of giving fourth dimension (i.e. comp) the name of the field, the name is assigned to the DataArray itself. The default name is 'field', but it can be changed with name parameter.
  4. The units of the field can be set with units parameter. Units of geometry dimensions are set to mesh.attributes['unit'] if the attribute exists, otherwise set to 'm' (meter).

Copy link
Member

@lang-m lang-m left a comment

Choose a reason for hiding this comment

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

Looks good. I left a few minor comments.

discretisedfield/field.py Outdated Show resolved Hide resolved
discretisedfield/field.py Outdated Show resolved Hide resolved
discretisedfield/field.py Outdated Show resolved Hide resolved
discretisedfield/field.py Outdated Show resolved Hide resolved
discretisedfield/field.py Outdated Show resolved Hide resolved
discretisedfield/field.py Outdated Show resolved Hide resolved
discretisedfield/field.py Outdated Show resolved Hide resolved
discretisedfield/field.py Show resolved Hide resolved
discretisedfield/field.py Show resolved Hide resolved
@lang-m
Copy link
Member

lang-m commented Feb 2, 2022

Please do ignore the failing pre-commit.ci. This isn't fully set up yet and therefore completely meaningless.

@codecov-commenter
Copy link

codecov-commenter commented Feb 2, 2022

Codecov Report

Merging #123 (44e58e5) into master (a20b19d) will increase coverage by 0.11%.
The diff coverage is 98.07%.

@@            Coverage Diff             @@
##           master     #123      +/-   ##
==========================================
+ Coverage   95.46%   95.58%   +0.11%     
==========================================
  Files          20       20              
  Lines        1986     2038      +52     
==========================================
+ Hits         1896     1948      +52     
  Misses         90       90              
Impacted Files Coverage Δ
discretisedfield/field.py 97.79% <98.07%> (+0.18%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a20b19d...44e58e5. Read the comment docs.

Check `field.components` is not `None` before assigning it as `comp`
co-ordinate values. Assigning `None` as a dimention co-ordinate throws
an error.
Copy link
Member

@marijanbeg marijanbeg left a comment

Choose a reason for hiding this comment

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

Great code @swapneelap. I added a few very minor comments.

discretisedfield/field.py Outdated Show resolved Hide resolved
discretisedfield/field.py Outdated Show resolved Hide resolved
discretisedfield/field.py Outdated Show resolved Hide resolved
@fangohr
Copy link
Member

fangohr commented Feb 3, 2022

This looks good.

Do we have tests for this method yet? If not, we need one. Martin or I can help to suggest some.

discretisedfield/field.py Show resolved Hide resolved
discretisedfield/field.py Outdated Show resolved Hide resolved
discretisedfield/field.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@swapneelap
Copy link
Member Author

This looks good.

Do we have tests for this method yet? If not, we need one. Martin or I can help to suggest some.

No, I have not added the tests yet. Yes, it's a good idea to discuss them!

discretisedfield/field.py Outdated Show resolved Hide resolved
discretisedfield/field.py Outdated Show resolved Hide resolved
discretisedfield/field.py Outdated Show resolved Hide resolved
discretisedfield/field.py Outdated Show resolved Hide resolved
discretisedfield/field.py Outdated Show resolved Hide resolved
discretisedfield/field.py Outdated Show resolved Hide resolved
discretisedfield/field.py Outdated Show resolved Hide resolved
discretisedfield/tests/test_field.py Show resolved Hide resolved
Copy link
Member

@marijanbeg marijanbeg left a comment

Choose a reason for hiding this comment

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

This looks good to me - great work! We can further simplify this code when @lang-m finishes implementing midpoints and vertices :)

swapneelap and others added 4 commits March 22, 2022 17:11
- Remove cells showing raised exceptions from `to_xarray` and
  `from_xarray` for better readability.

- Add `print()` to display `xarray.DataArray` as the html
  representation gives `pytest` errors with nbval.
To avoid problems with nbval all cells that contain xarray html output
are tagged with nbval-ignore-output. That way we do not compare the
actual html representation (which we don't need to) but we would still
notice when the cell execution fails.
Copy link
Member

@lang-m lang-m left a comment

Choose a reason for hiding this comment

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

The documentation looks good to me, it is very readable. I've removed the print for the xarray DataArrays to get the nicer-looking html representation and tagged the relevant cells to avoid failures of nbval.

I would suggest to keep this PR open until we can use the new mesh.midpoints.

@swapneelap
Copy link
Member Author

The documentation looks good to me, it is very readable. I've removed the print for the xarray DataArrays to get the nicer-looking html representation and tagged the relevant cells to avoid failures of nbval.

I would suggest to keep this PR open until we can use the new mesh.midpoints.

Thank you, that sounds good. I will update the methods when you merge the changes.

@swapneelap swapneelap merged commit e54e601 into master Mar 23, 2022
@swapneelap swapneelap deleted the field-to-xarray branch March 23, 2022 11:31
@marijanbeg
Copy link
Member

Very nice @swapneelap!

@fangohr
Copy link
Member

fangohr commented Mar 23, 2022

An important improvement - thank you!

@swapneelap
Copy link
Member Author

Thank you all for your reviews and comments! It helped in making the methods concise, readable, and efficient. :)

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.

None yet

5 participants