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

Boundary #30

Merged
merged 9 commits into from
Oct 18, 2023
Merged

Boundary #30

merged 9 commits into from
Oct 18, 2023

Conversation

rafa-guedes
Copy link
Collaborator

This pull request moves the old rompy.swan.boundary.DataBoundary into a new core object rompy.core.boundary.BoundaryWaveStation. The old swan boundary object is now called rompy.swan.boundary.Boundnest1 and inherits from BoundaryWaveStation with only small changes to the get method to create SWAN ASCII boundary files instead of netcdf.

Addresses #27.

@@ -929,8 +929,8 @@
"source": [
"# The boundary object work in a similar way but is a little more complex as it must determine points around the boundary of the model domain.\n",
"# First lets have a look at the boundary object\n",
"from rompy.swan import DataBoundary\n",
"DataBoundary?"
"from rompy.swan import Boundnest1\n",
Copy link
Member

Choose a reason for hiding this comment

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

Whilst the DataBoundary as an abstract class is new - I would argue that Boundnest1 is an inappropriate default boundary type for SWAN as this is mostly for nested swan simulations. Arguably most swan simulations will be nested within a global/regional model or forced with wave buoy data so BOUNDSPEC in most cases.

@pbranson
Copy link
Member

I have had a look, I think this all makes sense, but I wonder if the default boundary type for SWAN should be BOUNDSPEC rather than BOUNDNEST1.

@rafa-guedes
Copy link
Collaborator Author

rafa-guedes commented Oct 17, 2023

We can make BOUNDSPEC the default @pbranson, my idea was to allow generating the different boundary types by interfacing with the data objects, Boundnest1 only happened to be the first one to be implemented in this new approach. The way these objects are being defined might as well change if that makes more sense.

The main thing with this current pull request though is the separation of the previously-defined rompy.swan.boundary.DataBoundary into a rompy.core.boundary.BoundaryWaveStation part which interacts with "stations" dataset and interpolates the spectra, and a rompy.core.swan.Boundnest1 part, which modifies BoundaryWaveStation to produce boundary in swan ascii format for the boundnest1 command (which is what DataBoundary was doing before). We could then implement similar objects to generate BOUNDSPEC etc. This will also allow using the logic defined with BoundaryWaveStation with schism and other models more easily.

@rafa-guedes
Copy link
Collaborator Author

rafa-guedes commented Oct 17, 2023

@benjaminleighton would you be able to also review this and merge if happy please since you have been working with that boundary object? This implements some changes from DataBoundary.

@benjaminleighton
Copy link
Collaborator

Hi @rafa-guedes this looks good to me.

@benjaminleighton benjaminleighton merged commit fda6472 into main Oct 18, 2023
@rafa-guedes rafa-guedes deleted the boundary branch July 25, 2024 04:02
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