Skip to content

MeshCoord class [AVD-1591]#4036

Merged
bjlittle merged 10 commits intoSciTools:mesh-data-modelfrom
pp-mo:meshcoord
Mar 5, 2021
Merged

MeshCoord class [AVD-1591]#4036
bjlittle merged 10 commits intoSciTools:mesh-data-modelfrom
pp-mo:meshcoord

Conversation

@pp-mo
Copy link
Member

@pp-mo pp-mo commented Feb 25, 2021

🚀 Pull Request

Description

Adding MeshCoord.

Notes:

  1. the characteristic points and bounds indexing calculations are not yet implemented here.
    I have other code prototyped for that, but this PR is simpler if I leave it out for now.
  2. the implementation isn't quite minimal, but I have chosen to control copying and slicing to make it a 'good citizen' within a cube (i.e. not blocking cube copy or slicing).
  3. there is as yet no sensible __str__ or __repr__. We have another task pending to address that.

Consult Iris pull request check list

@pp-mo
Copy link
Member Author

pp-mo commented Feb 25, 2021

I think this is all that is needed, but you may well decide there is something missing @trexfeathers.
The testing is using specific real things rather than mock-ist, so you might well uncover some possibility I have missed due to that.

I am a little surprised there is not more to this, but that is probably because we have decided now to defer implementing the "bounds but no points" ability. Which I thought might be tricky, and now think even more so!

@stephenworsley it will be good if you can cast an eye over this, but I think @trexfeathers is steering at this point.

@pp-mo
Copy link
Member Author

pp-mo commented Mar 1, 2021

Update: I'm now working to reduce the __init__ args to just mesh/location/axis, and thus enforce the intended content wrt the provided args.

I'll "WIP" / draft this : please don't merge till those changes are up

@pp-mo pp-mo marked this pull request as draft March 1, 2021 09:58
@pp-mo
Copy link
Member Author

pp-mo commented Mar 1, 2021

Failing docs tests -- probably needs another rebase ?

Meantimes, since discussing with @trexfeathers today, I am now intending to fix the copy/init to support the cube slicing needs in a nicer way (i.e. without bodging the allowed copy args as at present).
Hence this is still WIP

Copy link
Contributor

@trexfeathers trexfeathers left a comment

Choose a reason for hiding this comment

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

I've been asked to stop reviewing this and work on something else. So here's what I've got so far. Mostly optional minor improvements, but I definitely think you should remove those setter methods (see individual comments). Thanks 🙂

@pp-mo pp-mo requested a review from bjlittle March 2, 2021 15:23
@pp-mo pp-mo marked this pull request as ready for review March 2, 2021 15:24
@pp-mo
Copy link
Member Author

pp-mo commented Mar 2, 2021

Latest:

  • rebased to fix docs test failures (I hope)
  • addressed all outstanding comments (I think)
  • revised the copy and __getitem__ overrides to support minimum needs, as promised above
  • added testing for the result points+bounds arrays (preliminary, pending "dynamic" versions of the calcs)

Please review @bjlittle !

Copy link
Member

@bjlittle bjlittle left a comment

Choose a reason for hiding this comment

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

@pp-mo Lovely stuff 👍

This is the super interesting part (for me) now... the interaction of MeshCoord, Mesh and Cube

Okay, so I've raised a few review questions and issues, but nothing controversial . I'm sure you can easily address them.

Otherwise, this is looking very promising 👍

@bjlittle bjlittle assigned bjlittle and unassigned trexfeathers Mar 4, 2021
@pp-mo
Copy link
Member Author

pp-mo commented Mar 4, 2021

Thanks @bjlittle
Rebased to use your recent addition of Mesh.LOCATIONS.
Hope I've addressed everything raised so far.
See if you still don't like the setters for coord_system + climatological . If you really dislike this, I guess we can do something different with these somehow, but it's a bit "tail wagging dog" to modify Coord for the benefit of the subclass !

@pp-mo
Copy link
Member Author

pp-mo commented Mar 5, 2021

@bjlittle apologies, forgot to push.
Please re-review 😁

@pp-mo
Copy link
Member Author

pp-mo commented Mar 5, 2021

Further changes as requested (?mostly).
Please re-view @bjlittle

I'm still slightly uncomfortable about the shortcut on __eq__ comparison, but we clearly don't want it comparing points+bounds also.

@pp-mo
Copy link
Member Author

pp-mo commented Mar 5, 2021

Fixed the equality thing.
As discussed offline.
Ok @bjlittle !

@bjlittle bjlittle merged commit be62cdf into SciTools:mesh-data-model Mar 5, 2021
@pp-mo pp-mo deleted the meshcoord branch March 18, 2022 14:54
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