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

(v0.6.0) Simplify boundary condition construction #175

Merged
merged 10 commits into from
Jul 2, 2024

Conversation

navidcy
Copy link
Contributor

@navidcy navidcy commented May 9, 2024

Description edited by Ashley to reflect discussion below
This PR aims to simplify the boundary condition construction in the simple case of four boundaries that are parallel to lines of constant longitude and latitude, as per the discussion in #174.

To do this, we add a wrapper function that just calls the simple_boundary method for each cardinal direction requested.

This PR includes a breaking API change since rectangular_boundary is renamed to rectangular_boundaries and also some of its args changed.

Closes #174

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@ashjbarnes
Copy link
Collaborator

ashjbarnes commented May 10, 2024

In this PR you're changing the initial condition function to also do the boundary right? I still think that the boundary and initial conditions should be kept as separate functions. If not, then we'll should rename it so that the "initial condition" function doesn't do the boundary right? This is confusing to me

Perhaps we copy - paste the 'iterate over boundaries' file to the setup_rectangular_boundary() function so that this function will default to four automatic boundaries, but user can just provide one if they want

@navidcy
Copy link
Contributor Author

navidcy commented May 10, 2024

Oh my bad

@ashjbarnes ashjbarnes changed the title Simplify initial condition construction Simplify boundary condition construction Jun 11, 2024
@ashjbarnes
Copy link
Collaborator

This PR will require a new tag release as it's not backwards compatible. Also need to update the access om2 example with the changes.

@ashjbarnes
Copy link
Collaborator

I'm going to approve the PR because it was Navid's originally. Wait on merging until @navidcy is happy and we also fix up the access om2 example to match simultaneously

@navidcy
Copy link
Contributor Author

navidcy commented Jul 1, 2024

I'd like to merge this and then we tag a new release -- how does this sound @ashjbarnes?

@navidcy navidcy changed the title Simplify boundary condition construction (v0.6.0) Simplify boundary condition construction Jul 1, 2024
@navidcy navidcy merged commit 4781919 into main Jul 2, 2024
6 checks passed
@navidcy navidcy deleted the ncc/friendlier-initial-condition branch July 2, 2024 03:01
@ashjbarnes
Copy link
Collaborator

Great! Need to reflect changes in access om2 example

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Iterate through boundaries as part of expt.initial_condition method
2 participants