Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
added functional timestamps #6125
base: main
Are you sure you want to change the base?
added functional timestamps #6125
Changes from 2 commits
13e7ee1
7b32abe
824c8ae
9abb8e2
96a2000
75a471f
378e642
6c2d9bf
56f994c
092e354
3cbe9c0
0fc0099
67c9ed0
d8ad4dd
a97a177
050d47b
9c0b61d
d584439
99c995a
ab3072b
adbb97b
73e0062
9b4f204
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something missed in #4757 and in previous discussions: the metadata such as
units
,standard_name
, etcetera is also subject to change.Thankfully it is quick to recalculate, so we don't need to protect it behind a date check. But we do need a way to make sure it is recalculated on the fly.
Metadata classes a tricksy and it's probably best if we don't make any changes there. Instead we can put
self._metadata_manager
behind a `@property' so that we can regenerate it every time before returning it:1
2
3
I don't think this will be necessary since I don't think there will be any calls to set
self._metadata_manager
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-initialising the instance every time is a very surprising thing to do. I'm not sure of the possible consequences of doing this. Have you looked this up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually forget known consequences. Even if there are none, a big problem is that the parent classes will have been written assuming that
__init__
will only be run once. Even if it works now, someone could break it any time by making a seemingly 'safe' change. It might even happen during an edit toMeshCoord
, since it's not obvious from looking atMeshCoord.__init__
that this is happening.Can you come up with an alternative?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've had a look through the parent 'stack' and I reckon if you can work out a way to update the points, the bounds, and the metadata (
MeshCoordMetadata
) then you'll be in the clear.