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

2265 rename split dyn stencil #2266

Closed
wants to merge 6 commits into from
Closed

Conversation

oakleybrunt
Copy link
Collaborator

@oakleybrunt oakleybrunt commented Aug 15, 2023

closes #2265

sub-issue of #2235

@codecov
Copy link

codecov bot commented Aug 15, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (5a77078) 99.85% compared to head (9fba2af) 99.85%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2266   +/-   ##
=======================================
  Coverage   99.85%   99.85%           
=======================================
  Files         339      340    +1     
  Lines       46027    46030    +3     
=======================================
+ Hits        45958    45961    +3     
  Misses         69       69           
Files Changed Coverage Δ
src/psyclone/domain/lfric/__init__.py 100.00% <100.00%> (ø)
src/psyclone/domain/lfric/lfric_stencil.py 100.00% <100.00%> (ø)
src/psyclone/dynamo0p3.py 99.82% <100.00%> (-0.01%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

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

Thanks for doing this @oakleybrunt. I think it raises the question about whether we even need a separate class for this so I will gather opinions and then we can decide what to do.

src/psyclone/domain/lfric/__init__.py Outdated Show resolved Hide resolved
src/psyclone/domain/lfric/lfric_stencil.py Outdated Show resolved Hide resolved
src/psyclone/domain/lfric/lfric_stencil.py Outdated Show resolved Hide resolved
src/psyclone/domain/lfric/lfric_stencil.py Outdated Show resolved Hide resolved
@@ -0,0 +1,86 @@
# -----------------------------------------------------------------------------
Copy link
Member

Choose a reason for hiding this comment

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

Looking at this class, it seems to me we could do without it and have a NamedTuple instead:

LFRicArgStencil = namedtuple("Symbol", "name extent extent_arg direction_arg")

I say this because none of the setters do any validation. Can @rupertford, @TeranIvy or @hiker see any potential problem with doing that? (e.g. is this class likely to need significant additional functionality in future?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@arporter, I think that the potential future functionality would be "more of the same" in the sense of more stencil types, but the underlying mechanism wouldn't change. If doing away with the class doesn't affect adding more stencil types then I'm fine with it!

@arporter arporter added question LFRic Issue relates to the LFRic domain and removed under review labels Sep 13, 2023
@arporter
Copy link
Member

I've assigned everyone who I think might care so I can get your attention :-) The question is, is everyone happy if we do away with having a separate class (and tests etc) for 'DynStencil' in favour of just having a namedtuple?

@arporter
Copy link
Member

Hi @oakleybrunt, don't do anything yet - in all likelihood we will delete the class entirely!

@oakleybrunt
Copy link
Collaborator Author

Hi @oakleybrunt, don't do anything yet - in all likelihood we will delete the class entirely!

I had worked my way down the list before reading all the comments!! When I got to your final comment about changing to a namedtuple I had already done the edits 😆 I thought I may as well commit and push them. Thanks!

@arporter
Copy link
Member

OK, no one has complained so I suggest we go with the namedtuple route. If you look in psyclone/domain/lfric/lfric_types.py you'll see a few examples. I suggest you cut a new branch for this in case it goes pear shaped and we decide that what you've done in this PR is actually better. (The new namedtuple can stay in the dynamo0p3.py file.)

@oakleybrunt
Copy link
Collaborator Author

@arporter What are we doing with this PR? Is this closed now?

@arporter
Copy link
Member

arporter commented Nov 8, 2023

This proposed change was replaced with the use of a dataclass in #2321.

@arporter arporter closed this Nov 8, 2023
@arporter arporter deleted the 2265_rename_split_DynStencil branch November 8, 2023 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LFRic Issue relates to the LFRic domain reviewed with actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Splitting and renaming DynStencil from src/psyclone/dynamo0p3.py
6 participants