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

Inline harmonic analysis #718

Merged
merged 2 commits into from
Sep 6, 2024

Conversation

c2xu
Copy link

@c2xu c2xu commented Sep 6, 2024

Set default HAready = False

Also made additional adjustment to ensure harmonic analysis is not activated if tide is not used in the model.

Set default HAready = False

Also made additional adjustment to ensure harmonic analysis is not
activated if tide is not used in the model.
@c2xu
Copy link
Author

c2xu commented Sep 6, 2024

This PR should fix issue #708

@kshedstrom
Copy link

Yes, thank you, that fixes my Supercritical case (and makes the Arctic hycom troubles something else entirely).

Copy link
Member

@marshallward marshallward left a comment

Choose a reason for hiding this comment

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

Thank you @c2xu, I'll merge this once it passes testing.

Copy link
Member

@marshallward marshallward left a comment

Choose a reason for hiding this comment

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

I'm looking over this module, and I'm a bit uncomfortable with the use of return in so many functions, particularly with respect to the various parameter tests. In most cases, one could have moved the logic outside the function and avoided the function call altogether.

I was also a bit surprised that HA_CSp is not only a pointer but also a local variable that conditionally points to CS%HA_CSp. That is, the null state is being used to control the logical flow.

I don't want to block this PR over those concerns, but it might be a good idea to review this design later when we have a chance.

@c2xu
Copy link
Author

c2xu commented Sep 6, 2024

I'm looking over this module, and I'm a bit uncomfortable with the use of return in so many functions, particularly with respect to the various parameter tests. In most cases, one could have moved the logic outside the function and avoided the function call altogether.

I was also a bit surprised that HA_CSp is not only a pointer but also a local variable that conditionally points to CS%HA_CSp. That is, the null state is being used to control the logical flow.

I don't want to block this PR over those concerns, but it might be a good idea to review this design later when we have a chance.

Thanks for the comments. In addition, this module does not have the restart capability. I tried to add the restart capability but then it requires significant modification to the structure of the module. I guess it would be better to take care of all these issues altogether in a major revision of this module in the future. For now, I'm going to create an open issue to remind myself the points we discussed here.

@marshallward
Copy link
Member

@marshallward marshallward merged commit 91eee52 into NOAA-GFDL:dev/gfdl Sep 6, 2024
10 checks passed
@c2xu c2xu deleted the c2xu/harmonic_analysis branch September 21, 2024 00:12
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